lkml.org 
[lkml]   [2022]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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.

    \
     
     \ /
      Last update: 2022-05-18 17:58    [W:3.321 / U:0.156 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site