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.
