On Wed, May 18, 2022 at 12:48 AM David Gow davidgow@google.com wrote:
On Tue, May 17, 2022 at 3:48 AM Daniel Latypov dlatypov@google.com wrote:
Context:
- kunit_kernel.py is importing kunit_parser.py just to use the print_with_timestamp() function
- the parser is directly printing to stdout, which will become an issue if we ever try to run multiple kernels in parallel
This patch introduces a kunit_printer.py file and migrates callers of kunit_parser.print_with_timestamp() to call kunit_printer.stdout.print_with_timestamp() instead.
Future changes: If we want to support showing results for parallel runs, we could then create new Printer's that don't directly write to stdout and refactor the code to pass around these Printer objects.
Signed-off-by: Daniel Latypov dlatypov@google.com
I agree that this will be useful down the line, as running multiple kernels in parallel is definitely something which could be useful. I know the original idea for that was to have multiple parsers, and just to combine the results they gave after the fact, but given that incremental output is so useful, I agree that this is the better path.
My only super-minor gripe (which I can live with) is that importing 'stdout' and using it as 'stdout.print_with_timestamp()' is a little confusing: I'd've assumed an stdout variable imported into the global namespace was sys.stdout, not a wrapper. Explicitly using kunit_printer.stdout would be a little clearer, IMO. Up to you, though.
I was initially writing it that way, but then the following pattern got super long
Old: print_with_timestamp(red("[ERROR]") + " some error")
New options: stdout.print_with_timestamp(stdout.red("[ERROR]") + " some error") kunit_printer.stdout.print_with_timestamp(kunit_printer.stdout.red("[ERROR]") + " some error")
But yeah, I see what you mean about potential confusion with sys.stdout. I couldn't think of a better (while still short name) for it. E.g. "default_printer", "stdout_printer", etc.
FWIW, I have a local patch that drops 99% of the direct uses of kunit_printer.stdout in the parser and passes around buffered printers. And in that case, the use of stdout becomes small enough that we could do `kunit_printer.stdout` w/o as much pain/noise.
But I have no plans of sending that out until we need it, since it muddies up the code quite a bit. And I don't have a clear idea of what the interface to parallel testing should look like, so that day is still far off.