Messages in this thread | | | Date | Thu, 10 Aug 2023 15:18:50 +0100 | Subject | Re: [PATCH v3 2/7] kunit: kunit-test: Add test cases for extending log buffer | From | Richard Fitzgerald <> |
| |
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.
| |