Re: [PATCH v3 2/7] kunit: kunit-test: Add test cases for extending log buffer

[Richard Fitzgerald]

On 9/8/23 22:10, Rae Moar wrote:
> On Wed, Aug 9, 2023 at 11:54 AM Richard Fitzgerald
> <rf@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > Add test cases for the dynamically-extending log buffer.
> >
> > kunit_log_init_frag_test() tests that kunit_init_log_frag() correctly
> > initializes new struct kunit_log_frag.
> >
> > kunit_log_extend_test_1() logs a series of numbered lines then tests
> > that the resulting log contains all the lines.
> >
> > kunit_log_extend_test_2() logs a large number of lines of varying length
> > to create many fragments, then tests that all lines are present.
> >
> > kunit_log_newline_test() has a new test to append a line that is exactly
> > the length of the available space in the current fragment and check that
> > the resulting log has a trailing '\n'.
> >
> > Signed-off-by: Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxx>
>
> Hello!
>
> These tests now pass for me. Thanks!
>
> I do have a few comments below mostly regarding comments and a few
> clarifying questions.
>
> -Rae

...

> > +static void kunit_log_init_frag_test(struct kunit *test)
> >   {
> > -       struct kunit_suite suite;
> >          struct kunit_log_frag *frag;
> >
> > -       suite.log = kunit_kzalloc(test, sizeof(*suite.log), GFP_KERNEL);
> > -       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, suite.log);
> > -       INIT_LIST_HEAD(suite.log);
> >          frag = kunit_kmalloc(test, sizeof(*frag), GFP_KERNEL);
> >          KUNIT_ASSERT_NOT_ERR_OR_NULL(test, frag);
> > +       memset(frag, 0x5a, sizeof(*frag));
> > +
>
> Why is the fragment getting filled here with memset? Should this be
> tested? Feel free to let me know, I'm just uncertain.

I'll add a comment in V4. It's to prove that kunit_init_log_frag()
really did change something. kzalloc() is no good for this because we
want to see that kunit_log_frag() zeroed buf[0].

...

> >          kunit_info(test, "Add newline\n");
> >          if (test->log) {
> >                  frag = list_first_entry(test->log, struct kunit_log_frag, list);
> >                  KUNIT_ASSERT_NOT_NULL_MSG(test, strstr(frag->buf, "Add newline\n"),
> >                          "Missing log line, full log:\n%s",
> > -                       get_concatenated_log(test, test->log));
> > +                       get_concatenated_log(test, test->log, NULL));
> >                  KUNIT_EXPECT_NULL(test, strstr(frag->buf, "Add newline\n\n"));
> > +
>
> Should this section of kunit_log_newline_test be separated into a new
> test? This test seems a bit long and seems to have two distinct
> sections?

Yes, it makes sense to add a separate test case for when newlines cause
the log to extend.

...

> Another potential idea is to rename these two tests to be
> kunit_log_extend_test() and kunit_log_rand_extend_test() instead to be
> more descriptive?

TBH I had trouble thinking of a short description. But I'll spend some
time thinking about naming.

...

> > +       do {
> > +               n = snprintf(line, sizeof(line),
> > +                            "The quick brown fox jumps over the lazy penguin %d\n", i);
> > +               KUNIT_ASSERT_LT(test, n, sizeof(line));
> > +               kunit_log_append(suite.log, line);
> > +               ++i;
> > +               len += n;
> > +       }  while (len < (sizeof(frag->buf) * 30));
>
> Are we trying to restrict the num_frags to less than 30? And then we
> could check that with a KUNIT_EXPECT? Currently, the num_frags are
> just above 30. That is ok too. I just was wondering if this was
> intentional? (Same as kunit_log_extend_test_2)

I'll comment on this in V4.
It's just trying to create "a lot" of data without assuming exactly
how kunit_log_append() breaks up the lines across fragments. I don't
want to have to keep changing this code if the fragmenting algorithm
changes slightly. So the idea is to generate "about 30" buffers worth.
I don't mind if it's a bit more, or a bit less. It's done this way,
instead of just counting how many fragments were created, to prevent
getting into an infinite loop if for some reason kunit_log_append()
fails to add fragments.

...

> > +       /* Build log line of varying content */
> > +       line[0] = '\0';
> > +       i = 0;
> > +       do {
> > +               char tmp[9];
> > +
> > +               snprintf(tmp, sizeof(tmp), "%x", i++);
> > +               len = strlcat(line, tmp, sizeof(line));
> > +       } while (len < sizeof(line) - 1);
>
> Could there be an expectation statement here to check the line has
> been properly filled. Maybe checking the length?

Yes

> > +       prandom_seed_state(&rnd, 3141592653589793238ULL);
> > +       i = 0;
> > +       n = 0;
> > +       while ((pn = strchr(p, '\n')) != NULL) {
> > +               *pn = '\0';
> > +               KUNIT_EXPECT_STREQ(test, p, &line[i]);
> > +               p = pn + 1;
> > +               n++;
> > +               i = prandom_u32_state(&rnd) % (sizeof(line) - 1);
> > +       }
> > +       KUNIT_EXPECT_EQ_MSG(test, n, num_lines, "Not enough lines.");
>
> Is it possible for this to be too many lines instead? Should this
> comment instead be "Unexpected number of lines". Also could we have a
> similar message for the test above for this expectation regarding the
> number of lines.

Fair point. It's only found that the number of lines is wrong, it
could be less or more.