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>
---
lib/kunit/kunit-test.c | 182 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 174 insertions(+), 8 deletions(-)
diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
index a199f83bac67..c0ee33a8031e 100644
--- a/lib/kunit/kunit-test.c
+++ b/lib/kunit/kunit-test.c
@@ -7,6 +7,7 @@
*/
#include <kunit/test.h>
#include <kunit/test-bug.h>
+#include <linux/prandom.h>
#include "try-catch-impl.h"
@@ -530,10 +531,12 @@ static struct kunit_suite kunit_resource_test_suite = {
.test_cases = kunit_resource_test_cases,
};
-static char *get_concatenated_log(struct kunit *test, const struct list_head *log)
+static char *get_concatenated_log(struct kunit *test, const struct list_head *log,
+ int *num_frags)
{
struct kunit_log_frag *frag;
size_t len = 0;
+ int frag_count = 0;
char *p;
list_for_each_entry(frag, log, list)
@@ -542,24 +545,42 @@ static char *get_concatenated_log(struct kunit *test, const struct list_head *lo
len++; /* for terminating '\0' */
p = kunit_kzalloc(test, len, GFP_KERNEL);
- list_for_each_entry(frag, log, list)
+ list_for_each_entry(frag, log, list) {
strlcat(p, frag->buf, len);
+ ++frag_count;
+ }
+
+ if (num_frags)
+ *num_frags = frag_count;
return p;
}
-static void kunit_log_test(struct kunit *test)
+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));
+
kunit_init_log_frag(frag);
KUNIT_EXPECT_EQ(test, frag->buf[0], '\0');
+ KUNIT_EXPECT_TRUE(test, list_is_first(&frag->list, &frag->list));
+ KUNIT_EXPECT_TRUE(test, list_is_last(&frag->list, &frag->list));
+}
+
+static void kunit_log_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_kzalloc(test, sizeof(*frag), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, frag);
+ kunit_init_log_frag(frag);
list_add_tail(&frag->list, suite.log);
kunit_log(KERN_INFO, test, "put this in log.");
@@ -586,23 +607,168 @@ static void kunit_log_test(struct kunit *test)
static void kunit_log_newline_test(struct kunit *test)
{
+ struct kunit_suite suite;
struct kunit_log_frag *frag;
+ char *p;
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"));
+
+ 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_kzalloc(test, sizeof(*frag), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, frag);
+ kunit_init_log_frag(frag);
+ list_add_tail(&frag->list, suite.log);
+
+ /* String that exactly fills fragment leaving no room for \n */
+ memset(frag->buf, 0, sizeof(frag->buf));
+ memset(frag->buf, 'x', sizeof(frag->buf) - 9);
+ kunit_log_append(suite.log, "12345678");
+ p = get_concatenated_log(test, suite.log, NULL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, p);
+ KUNIT_EXPECT_NOT_NULL_MSG(test, strstr(p, "x12345678\n"),
+ "Newline not appended when fragment is full. Log is:\n'%s'", p);
} else {
kunit_skip(test, "only useful when debugfs is enabled");
}
}
+static void kunit_log_extend_test_1(struct kunit *test)
+{
+#ifdef CONFIG_KUNIT_DEBUGFS
+ struct kunit_suite suite;
+ struct kunit_log_frag *frag;
+ char line[60];
+ char *p, *pn;
+ size_t len, n;
+ int num_lines, num_frags, i;
+
+ 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_kzalloc(test, sizeof(*frag), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, frag);
+ kunit_init_log_frag(frag);
+ list_add_tail(&frag->list, suite.log);
+
+ i = 0;
+ len = 0;
+ 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));
+ num_lines = i;
+
+ p = get_concatenated_log(test, suite.log, &num_frags);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, p);
+ KUNIT_EXPECT_GT(test, num_frags, 1);
+
+ kunit_info(test, "num lines:%d num_frags:%d total len:%zu\n",
+ num_lines, num_frags, strlen(p));
+
+ i = 0;
+ while ((pn = strchr(p, '\n')) != NULL) {
+ *pn = '\0';
+ snprintf(line, sizeof(line),
+ "The quick brown fox jumps over the lazy penguin %d", i);
+ KUNIT_EXPECT_STREQ(test, p, line);
+ p = pn + 1;
+ ++i;
+ }
+ KUNIT_EXPECT_EQ(test, i, num_lines);
+#else
+ kunit_skip(test, "only useful when debugfs is enabled");
+#endif
+}
+
+static void kunit_log_extend_test_2(struct kunit *test)
+{
+#ifdef CONFIG_KUNIT_DEBUGFS
+ struct kunit_suite suite;
+ struct kunit_log_frag *frag;
+ struct rnd_state rnd;
+ char line[101];
+ char *p, *pn;
+ size_t len;
+ int num_lines, num_frags, n, i;
+
+ 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_kzalloc(test, sizeof(*frag), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, frag);
+ kunit_init_log_frag(frag);
+ list_add_tail(&frag->list, suite.log);
+
+ /* 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);
+
+ /*
+ * Log lines of different lengths until we have created
+ * many fragments.
+ * The "randomness" must be repeatable.
+ */
+ prandom_seed_state(&rnd, 3141592653589793238ULL);
+ i = 0;
+ len = 0;
+ num_lines = 0;
+ do {
+ kunit_log_append(suite.log, "%s\n", &line[i]);
+ len += sizeof(line) - i;
+ num_lines++;
+ i = prandom_u32_state(&rnd) % (sizeof(line) - 1);
+ } while (len < (sizeof(frag->buf) * 30));
+
+ /* There must be more than one buffer fragment now */
+ KUNIT_EXPECT_FALSE(test, list_is_singular(suite.log));
+
+ p = get_concatenated_log(test, suite.log, &num_frags);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, p);
+ KUNIT_EXPECT_GT(test, num_frags, 1);
+
+ kunit_info(test, "num lines:%d num_frags:%d total len:%zu\n",
+ num_lines, num_frags, strlen(p));
+
+ 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.");
+#else
+ kunit_skip(test, "only useful when debugfs is enabled");
+#endif
+}
+
static struct kunit_case kunit_log_test_cases[] = {
+ KUNIT_CASE(kunit_log_init_frag_test),
KUNIT_CASE(kunit_log_test),
KUNIT_CASE(kunit_log_newline_test),
+ KUNIT_CASE(kunit_log_extend_test_1),
+ KUNIT_CASE(kunit_log_extend_test_2),
{}
};
--
2.30.2On 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
> ---
> lib/kunit/kunit-test.c | 182 +++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 174 insertions(+), 8 deletions(-)
>
> diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
> index a199f83bac67..c0ee33a8031e 100644
> --- a/lib/kunit/kunit-test.c
> +++ b/lib/kunit/kunit-test.c
> @@ -7,6 +7,7 @@
> */
> #include <kunit/test.h>
> #include <kunit/test-bug.h>
> +#include <linux/prandom.h>
>
> #include "try-catch-impl.h"
>
> @@ -530,10 +531,12 @@ static struct kunit_suite kunit_resource_test_suite = {
> .test_cases = kunit_resource_test_cases,
> };
>
> -static char *get_concatenated_log(struct kunit *test, const struct list_head *log)
> +static char *get_concatenated_log(struct kunit *test, const struct list_head *log,
> + int *num_frags)
> {
> struct kunit_log_frag *frag;
> size_t len = 0;
> + int frag_count = 0;
> char *p;
>
> list_for_each_entry(frag, log, list)
> @@ -542,24 +545,42 @@ static char *get_concatenated_log(struct kunit *test, const struct list_head *lo
> len++; /* for terminating '\0' */
> p = kunit_kzalloc(test, len, GFP_KERNEL);
>
> - list_for_each_entry(frag, log, list)
> + list_for_each_entry(frag, log, list) {
> strlcat(p, frag->buf, len);
> + ++frag_count;
> + }
> +
> + if (num_frags)
> + *num_frags = frag_count;
>
> return p;
> }
>
> -static void kunit_log_test(struct kunit *test)
> +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.
> kunit_init_log_frag(frag);
> KUNIT_EXPECT_EQ(test, frag->buf[0], '\0');
> + KUNIT_EXPECT_TRUE(test, list_is_first(&frag->list, &frag->list));
> + KUNIT_EXPECT_TRUE(test, list_is_last(&frag->list, &frag->list));
> +}
> +
> +static void kunit_log_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_kzalloc(test, sizeof(*frag), GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, frag);
> + kunit_init_log_frag(frag);
> list_add_tail(&frag->list, suite.log);
>
> kunit_log(KERN_INFO, test, "put this in log.");
> @@ -586,23 +607,168 @@ static void kunit_log_test(struct kunit *test)
>
> static void kunit_log_newline_test(struct kunit *test)
> {
> + struct kunit_suite suite;
> struct kunit_log_frag *frag;
> + char *p;
Similar to last email, could we change p to be a more descriptive name
such as concat_log?
>
> 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?
> + suite.log = kunit_kzalloc(test, sizeof(*suite.log), GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, suite.log);
> + INIT_LIST_HEAD(suite.log);
I would love to see a comment here to explain and break up this
section similar to the comment from the previous email.
> + frag = kunit_kzalloc(test, sizeof(*frag), GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, frag);
> + kunit_init_log_frag(frag);
> + list_add_tail(&frag->list, suite.log);
> +
> + /* String that exactly fills fragment leaving no room for \n */
> + memset(frag->buf, 0, sizeof(frag->buf));
> + memset(frag->buf, 'x', sizeof(frag->buf) - 9);
> + kunit_log_append(suite.log, "12345678");
> + p = get_concatenated_log(test, suite.log, NULL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, p);
> + KUNIT_EXPECT_NOT_NULL_MSG(test, strstr(p, "x12345678\n"),
> + "Newline not appended when fragment is full. Log is:\n'%s'", p);
> } else {
> kunit_skip(test, "only useful when debugfs is enabled");
> }
> }
>
> +static void kunit_log_extend_test_1(struct kunit *test)
In general, I would really like to see more comments in the next two
tests describing the test behavior. I would prefer a comment for each
of the while/do-while loops below. I just found the behavior to be
slightly confusing to understand without comments (although I do
appreciate the comments that are in kunit_log_extend_test_2).
Also, I really appreciate how detailed these tests are.
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?
> +{
> +#ifdef CONFIG_KUNIT_DEBUGFS
> + struct kunit_suite suite;
> + struct kunit_log_frag *frag;
> + char line[60];
> + char *p, *pn;
Similar to before, could we change p and pn to be slightly more
descriptive names? Maybe concat_log and newline_ptr or newline_log or
newline_char?
> + size_t len, n;
> + int num_lines, num_frags, i;
> +
> + 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_kzalloc(test, sizeof(*frag), GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, frag);
> + kunit_init_log_frag(frag);
> + list_add_tail(&frag->list, suite.log);
> +
> + i = 0;
> + len = 0;
> + 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)
> + num_lines = i;
> +
> + p = get_concatenated_log(test, suite.log, &num_frags);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, p);
> + KUNIT_EXPECT_GT(test, num_frags, 1);
> +
> + kunit_info(test, "num lines:%d num_frags:%d total len:%zu\n",
> + num_lines, num_frags, strlen(p));
> +
> + i = 0;
> + while ((pn = strchr(p, '\n')) != NULL) {
> + *pn = '\0';
> + snprintf(line, sizeof(line),
> + "The quick brown fox jumps over the lazy penguin %d", i);
> + KUNIT_EXPECT_STREQ(test, p, line);
> + p = pn + 1;
> + ++i;
> + }
> + KUNIT_EXPECT_EQ(test, i, num_lines);
> +#else
> + kunit_skip(test, "only useful when debugfs is enabled");
> +#endif
> +}
> +
> +static void kunit_log_extend_test_2(struct kunit *test)
> +{
> +#ifdef CONFIG_KUNIT_DEBUGFS
> + struct kunit_suite suite;
> + struct kunit_log_frag *frag;
> + struct rnd_state rnd;
> + char line[101];
> + char *p, *pn;
Similar to above, could p and pn be renamed to be more descriptive?
> + size_t len;
> + int num_lines, num_frags, n, i;
> +
> + 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_kzalloc(test, sizeof(*frag), GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, frag);
> + kunit_init_log_frag(frag);
> + list_add_tail(&frag->list, suite.log);
> +
> + /* 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?
> +
> + /*
> + * Log lines of different lengths until we have created
> + * many fragments.
> + * The "randomness" must be repeatable.
> + */
> + prandom_seed_state(&rnd, 3141592653589793238ULL);
> + i = 0;
> + len = 0;
> + num_lines = 0;
> + do {
> + kunit_log_append(suite.log, "%s\n", &line[i]);
> + len += sizeof(line) - i;
> + num_lines++;
> + i = prandom_u32_state(&rnd) % (sizeof(line) - 1);
> + } while (len < (sizeof(frag->buf) * 30));
> +
> + /* There must be more than one buffer fragment now */
> + KUNIT_EXPECT_FALSE(test, list_is_singular(suite.log));
> +
> + p = get_concatenated_log(test, suite.log, &num_frags);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, p);
> + KUNIT_EXPECT_GT(test, num_frags, 1);
> +
> + kunit_info(test, "num lines:%d num_frags:%d total len:%zu\n",
> + num_lines, num_frags, strlen(p));
> +
> + 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.
> +#else
> + kunit_skip(test, "only useful when debugfs is enabled");
> +#endif
> +}
> +
> static struct kunit_case kunit_log_test_cases[] = {
> + KUNIT_CASE(kunit_log_init_frag_test),
> KUNIT_CASE(kunit_log_test),
> KUNIT_CASE(kunit_log_newline_test),
> + KUNIT_CASE(kunit_log_extend_test_1),
> + KUNIT_CASE(kunit_log_extend_test_2),
> {}
> };
>
> --
> 2.30.2
>
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.
© 2016 - 2025 Red Hat, Inc.