Messages in this thread | | | From | Daniel Latypov <> | Date | Wed, 18 May 2022 08:50:48 -0700 | Subject | Re: [PATCH 3/3] kunit: tool: refactoring printing logic into kunit_printer.py |
| |
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.
| |