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

[Brendan Higgins]

On Thu, Jul 18, 2019 at 12:22:33PM -0700, Brendan Higgins wrote:
> On Thu, Jul 18, 2019 at 10:50 AM Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
> >
> > Quoting Brendan Higgins (2019-07-16 11:52:01)
> > > On Tue, Jul 16, 2019 at 10:50 AM Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
[...]
> > Do you have a link to those earlier patches?
> 
> This is the first patchset:
> 
> https://www.mail-archive.com/linux-kernel@xxxxxxxxxxxxxxx/msg1788057.html
> 
> In particular you can see the code for matching functions here:
> 
> https://www.mail-archive.com/linux-kernel@xxxxxxxxxxxxxxx/msg1788073.html
> 
> And parameter matching code here:
> 
> https://www.mail-archive.com/linux-kernel@xxxxxxxxxxxxxxx/msg1788072.html
> 
> https://www.mail-archive.com/linux-kernel@xxxxxxxxxxxxxxx/msg1788086.html
> 
> My apologies in advance, but the code at this early stage had not
> adopted the kunit_* prefix and was still using the test_* and mock_*
> prefix. (Hence, struct kunit_stream was known as struct test_stream).
[...]
> > The crux of my complaint is that the string stream API is too loosely
> > defined to be usable. It allows tests to build up a string of
> > unstructured information, but with certain calling constraints so we
> > have to tread carefully. If there was more structure to the data that's
> > being recorded then the test case runner could operate on the data
> > without having to do string/stream operations, allocations, etc. This
> > would make the assertion logic much more concrete and specific to kunit,
> > instead of this small kunit wrapper that's been placed on top of string
> > stream.
> 
> Yeah, I can see the point of wanting something that provides more
> structure than the raw `struct kunit_stream` interface. In fact, it is
> something I had already started working on, when I had determined it
> would be a large effort to capture all the variations. I was further
> put off from the idea when I had been asked to convert the KUnit
> intermediate format from what I was using to TAP, because, as it is,
> the current data printed out by KUnit doesn't contain all the data I
> would like to put in it in a way that best takes advantage of the TAP
> specification. One problematic area in particular: TAP already
> provides a way to present a lot of the data I would like to export,
> but it involves JSON serialization which was an idea that some of the
> other reviewers understandably weren't too keen on. TAP also wants to
> report data some time after it is available, which is generally not a
> good idea for test debug information; you want to make it available as
> soon as you can or you risk crashing with the data still inside.
> 
> Hence, I decided we could probably spend a good long while debating
> how I present the information. So the idea of having a loose
> definition seemed attractive to me in its own right since it would
> likely conform to whatever we ended up deciding in the long run. Also,
> all the better that it was what I already had and no one seemed to
> mind too much.
> 
> The only constant I expect is that `struct kunit` will likely need to
> take an abstract object with a `commit` method, or a `format` method
> or whatever so it could control when data was going to be printed out
> to the user. We will probably also use a string builder in there
> somewhere.
> 
> > TL;DR: If we can get rid of the string stream API I'd view that as an
> > improvement because building arbitrary strings in the kernel is complex,
> > error prone and has calling context concerns.
> 
> True. No argument there.
> 
> > Is the intention that other code besides unit tests will use this string
> > stream API to build up strings? Any targets in mind? This would be a
> > good way to get the API merged upstream given that its 2019 and we
> > haven't had such an API in the kernel so far.
> 
> Someone, (was it you?) asked about code sharing with a string builder
> thingy that was used for creating structured human readable files, but
> that seemed like a pretty massive undertaking.
> 
> Aside from that, no. I would kind of prefered that nobody used it for
> anything else because I the issues you described.
> 
> Nevertheless, I think the debate over the usefulness of the
> string_stream and kunit_stream are separate topics. Even if we made
> kunit_stream more structured, I am pretty sure I would want to use
> string_stream or some variation for constructing the message.
> 
> > An "object oriented" (strong quotes!) approach where kunit_fail_msg is
> > the innermost struct in some assertion specific structure might work
> > nicely and allow the test runner to call a generic 'format' function to
> > print out the message based on the type of assertion/expectation it is.
> > It probably would mean less code size too because the strings that are
> > common will be in the common printing function instead of created twice,
> > in the macros/code and then copied to the heap for the string stream.
> >
> >         struct kunit_assert {
> >                 const char *line;
> >                 const char *file;
> >                 const char *func;
> >                 void (*format)(struct kunit_assert *assert);
> >         };
> >
> >         struct kunit_comparison_assert {
> >                 enum operator operator;
> >                 const char *left;
> >                 const char *right;
> >                 struct kunit_assert assert;
> >         };
> >
> >         struct kunit_bool_assert {
> >                 const char *truth;
> >                 const char *statement;
> >                 struct kunit_assert assert;
> >         };
> >
> >         void kunit_format_comparison(struct kunit_assert *assert)
> >         {
> >                 struct kunit_comparison_assert *comp = container_of(assert, ...)
> >
> >                 kunit_printk(...)
> >         }

I started poking around with your suggestion while we are waiting. A
couple early observations:

1) It is actually easier to do than I previously thought and will probably
   help with getting more of the planned TAP output stuff working.

   That being said, this is still a pretty substantial undertaking and
   will likely take *at least* a week to implement and properly review.
   Assuming everything goes extremely well (no unexpected issues on my
   end, very responsive reviewers, etc).

2) It *will* eliminate the need for kunit_stream.

3) ...but, it *will not* eliminate the need for string_stream.

Based on my early observations, I do think it is worth doing, but I
don't think it is worth trying to make it in this patchset (unless I
have already missed the window, or it is going to be open for a while):
I do think it will make things much cleaner, but I don't think it will
achieve your desired goal of getting rid of an unstructured
{kunit|string}_stream style interface; it just adds a layer on top of it
that makes it harder to misuse.

I attached a patch of what I have so far at the end of this email so you
can see what I am talking about. And of course, if you agree with my
assessment, so we can start working on it as a future patch.

A couple things in regard to the patch I attached:

1) I wrote it pretty quickly so there are almost definitely mistakes.
   You should consider it RFC. I did verify it compiles though.

2) Also, I did use kunit_stream in writing it: all occurences should be
   pretty easy to replace with string_stream; nevertheless, the reason
   for this is just to make it easier to play with the current APIs. I
   wanted to have something working before I went through a big tedious
   refactoring. So sorry if it causes any confusion.

3) I also based the patch on all the KUnit patches I have queued up
   (includes things like mocking and such) since I want to see how this
   serialization thing will work with mocks and matchers and things like
   that.

> I started working on something similarish, but by the time I ended up
> coming up with a parent object whose definition was loose enough to
> satisfy all the properties required by the child classes it ended up
> basically being the same as what I have now just with a more complex
> hierarchy of message manipulation logic.
> 
> On the other hand, I didn't have the idea of doing the parent object
> quite the way you did and that would clean up a lot of the duplicated
> first line logic.
> 
> I would like to give it a try, but I am afraid I am going to get
> sucked down a really deep rabbit hole.
> 
> > Maybe other people have opinions here on if you should do it now or
> > later. Future coding is not a great argument because it's hard to
> > predict the future. On the other hand, this patchset is in good shape to
> 
> Yeah, that's kind of why I am afraid to go down this road when I have
> something that works now and I know works with the mocking stuff I
> want to do.
> 
> I would like to try your suggestion, but I want to try to make it work
> with my mocking patches before I commit to it because otherwise I am
> just going to have to back it out in a follow up patchset.
> 
> > merge and I'd like to use it to write unit tests for code I maintain so
> > I don't want to see this stall out. Sorry if I'm opening the can of
> > worms you're talking about.
> 
> Don't be sorry. I agree with you that the kunit_stream stuff is not very pretty.
> 
> Shuah, have we missed the merge window for 5.3?
> 
> I saw you only sent one PR out so far for this release, and there
> wasn't much in it; I imagine you are going to send at least one more?
> 
> I figure, if we still got time to try out your suggestion, Stephen, no
> harm in trying.
> 
> Also if we missed it, then I have another couple months to play around with it.
> 
> What do you think?

I attached the patch mentioned above below. Let me know what you think!

Cheers!