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.