On Wed, May 18, 2022 at 11:51 AM Daniel Latypov <dlatypov@xxxxxxxxxx> wrote:
>
> On Wed, May 18, 2022 at 12:48 AM David Gow <davidgow@xxxxxxxxxx> wrote:
> >
> > On Tue, May 17, 2022 at 3:48 AM Daniel Latypov <dlatypov@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> >
> > 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")
Kind of late to mention this (and we might have already talked about
this offline), but I am fine with what you have done here with the
stdout.
My initial reaction was similar to David's, but after thinking about
it, I don't think it is prone to misuse, and I think it is clear - and
allows for easy refactoring in the future.
> 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.
