Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger

[Stephen Boyd]

Quoting Brendan Higgins (2019-07-16 01:37:34)
> On Tue, Jul 16, 2019 at 12:57 AM Brendan Higgins
> <brendanhiggins@xxxxxxxxxx> wrote:
> >
> > On Mon, Jul 15, 2019 at 3:15 PM Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
> > >
> > > Quoting Brendan Higgins (2019-07-12 01:17:30)
> > > > diff --git a/include/kunit/kunit-stream.h b/include/kunit/kunit-stream.h
> > > > new file mode 100644
> > > > index 0000000000000..a7b53eabf6be4
> > > > --- /dev/null
> > > > +++ b/include/kunit/kunit-stream.h
> > > > +/**
> > > > + * struct kunit_stream - a std::stream style string builder.
> > > > + *
> > > > + * A std::stream style string builder. Allows messages to be built up and
> > > > + * printed all at once.
> > > > + */
> > > > +struct kunit_stream {
> > > > +       /* private: internal use only. */
> > > > +       struct kunit *test;
> > > > +       const char *level;
> > >
> > > Is the level changed? See my comment below, but I wonder if this whole
> > > struct can go away and the wrappers can just operate on 'struct
> > > string_stream' instead.
> >
> > I was inclined to agree with you when I first read your comment, but
> > then I thought about the case that someone wants to add in a debug
> > message (of which I currently have none). I think under most
> > circumstances a user of kunit_stream would likely want to pick a
> > default verbosity that maybe I should provide, but may still want
> > different verbosity levels.
> >
> > The main reason I want to keep the types separate, string_stream vs.
> > kunit_stream, is that they are intended to be used differently.
> > string_stream is just a generic string builder. If you are using that,
> > you are expecting to see someone building the string at some point and
> > then doing something interesting with it. kunit_stream really tells
> > you specifically that KUnit is putting together a message to
> > communicate something to a user of KUnit. It is really used in a very
> > specific way, and I wouldn't want to generalize its usage beyond how
> > it is currently used. I think in order to preserve the author's
> > intention it adds clarity to keep the types separate regardless of how
> > similar they might be in reality.

You may want to add some of these reasons to the commit text.

> > > > +
> > > > +       if (!string_stream_is_empty(stream->internal_stream)) {
> > > > +               kunit_err(stream->test,
> > > > +                         "End of test case reached with uncommitted stream entries\n");
> > > > +               kunit_stream_commit(stream);
> > > > +       }
> > > > +}
> > > > +
> > >
> > > Nitpick: Drop this extra newline.
> >
> > Oops, nice catch.
> 
> Not super important, but I don't want you to think that I am ignoring
> you. I think you must have unintentionally deleted the last function
> in this file, or maybe you are referring to something that I am just
> not seeing, but I don't see the extra newline here.

No worries. Sorry for the noise.

> > property of the input, it may or may not be enough information on its
> > own for the expectation to fail, but we want to share the result of
> > the property check with the user regardless, BUT only if the
> > expectation as a whole fails.
> >
> > Hence, we can have multiple `struct kunit_stream`s associated with a
> > `struct kunit` active at any given time.

Makes sense. I wasn't sure if there were more than one stream associated
with a test. Sounds like there are many to one so it can't just be a
member of the test. This could be documented somewhere so this question
doesn't come up again.