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

Richard Fitzgerald
2023-08-10T14:19:12Z

On 9/8/23 22:10, Rae Moar wrote:
> On Wed, Aug 9, 2023 at 11:54 AM Richard Fitzgerald
> <rf@opensource.cirrus.com> 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@opensource.cirrus.com>
>
> 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.