Re: [PATCH v4 01/10] kunit: string-stream: Improve testing of string_stream

[David Gow]

On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald
<rf@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> Replace the minimal tests with more-thorough testing.
>
> string_stream_init_test() tests that struct string_stream is
> initialized correctly.
>
> string_stream_line_add_test() adds a series of numbered lines and
> checks that the resulting string contains all the lines.
>
> string_stream_variable_length_line_test() adds a large number of
> lines of varying length to create many fragments, then tests that all
> lines are present.
>
> string_stream_append_test() tests various cases of using
> string_stream_append() to append the content of one stream to another.
>
> Signed-off-by: Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxx>
> ---

Thanks. These tests are much better than the original string stream ones.

It looks good to me. I left a few notes below (mostly to myself), but
nothing that would require a new version by itself.

Reviewed-by: David Gow <davidgow@xxxxxxxxxx>

Cheers,
-- David

>  lib/kunit/string-stream-test.c | 200 ++++++++++++++++++++++++++++++---
>  1 file changed, 184 insertions(+), 16 deletions(-)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 110f3a993250..1d46d5f06d2a 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -11,38 +11,206 @@
>
>  #include "string-stream.h"
>
> -static void string_stream_test_empty_on_creation(struct kunit *test)
> +/* string_stream object is initialized correctly. */
> +static void string_stream_init_test(struct kunit *test)
>  {
> -       struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL);
> +       struct string_stream *stream;
> +
> +       stream = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
> +
> +       KUNIT_EXPECT_EQ(test, stream->length, 0);
> +       KUNIT_EXPECT_TRUE(test, list_empty(&stream->fragments));
> +       KUNIT_EXPECT_PTR_EQ(test, stream->test, test);
> +       KUNIT_EXPECT_EQ(test, stream->gfp, GFP_KERNEL);

As the kernel test robot notes, this may trigger a sparse warning, as
KUnit stores integer types as 'long long' internally in assertions.
Ignore it for now, we'll see if we can fix it in KUnit.

>
>         KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
>  }
>
> -static void string_stream_test_not_empty_after_add(struct kunit *test)
> +/*
> + * Add a series of lines to a string_stream. Check that all lines
> + * appear in the correct order and no characters are dropped.
> + */
> +static void string_stream_line_add_test(struct kunit *test)
>  {
> -       struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL);
> +       struct string_stream *stream;
> +       char line[60];
> +       char *concat_string, *pos, *string_end;
> +       size_t len, total_len;
> +       int num_lines, i;
>
> -       string_stream_add(stream, "Foo");
> +       stream = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
>
> -       KUNIT_EXPECT_FALSE(test, string_stream_is_empty(stream));
> +       /* Add series of sequence numbered lines */
> +       total_len = 0;
> +       for (i = 0; i < 100; ++i) {
> +               len = snprintf(line, sizeof(line),
> +                       "The quick brown fox jumps over the lazy penguin %d\n", i);

Assuming the 'd' in '%d' counts, this still has every letter of the
alphabet in it. :-)

> +
> +               /* Sanity-check that our test string isn't truncated */
> +               KUNIT_ASSERT_LT(test, len, sizeof(line));
> +
> +               string_stream_add(stream, line);
> +               total_len += len;
> +       }
> +       num_lines = i;
> +
> +       concat_string = string_stream_get_string(stream);
> +       KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string);
> +       KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len);
> +
> +       /*
> +        * Split the concatenated string at the newlines and check that
> +        * all the original added strings are present.
> +        */
> +       pos = concat_string;
> +       for (i = 0; i < num_lines; ++i) {
> +               string_end = strchr(pos, '\n');
> +               KUNIT_EXPECT_NOT_NULL(test, string_end);
> +
> +               /* Convert to NULL-terminated string */
> +               *string_end = '\0';
> +
> +               snprintf(line, sizeof(line),
> +                        "The quick brown fox jumps over the lazy penguin %d", i);
> +               KUNIT_EXPECT_STREQ(test, pos, line);
> +
> +               pos = string_end + 1;
> +       }
> +
> +       /* There shouldn't be any more data after this */
> +       KUNIT_EXPECT_EQ(test, strlen(pos), 0);
>  }
>
> -static void string_stream_test_get_string(struct kunit *test)
> +/* Add a series of lines of variable length to a string_stream. */
> +static void string_stream_variable_length_line_test(struct kunit *test)
>  {
> -       struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL);
> -       char *output;
> +       static const char line[] =
> +               "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"
> +               " 0123456789!$%^&*()_-+={}[]:;@'~#<>,.?/|";
> +       struct string_stream *stream;
> +       struct rnd_state rnd;
> +       char *concat_string, *pos, *string_end;
> +       size_t offset, total_len;
> +       int num_lines, i;
>
> -       string_stream_add(stream, "Foo");
> -       string_stream_add(stream, " %s", "bar");
> +       stream = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
>
> -       output = string_stream_get_string(stream);
> -       KUNIT_ASSERT_STREQ(test, output, "Foo bar");
> +       /*
> +        * Log many lines of varying lengths until we have created
> +        * many fragments.
> +        * The "randomness" must be repeatable.
> +        */
> +       prandom_seed_state(&rnd, 3141592653589793238ULL);

This being deterministic is good. There are a few tests which are
doing similar things with randomness, and I think we'll want to have a
shared framework for it at some point (to enable a 'fuzzing' mode
which is _not_ deterministic), but this is good for now.

> +       total_len = 0;
> +       for (i = 0; i < 100; ++i) {
> +               offset = prandom_u32_state(&rnd) % (sizeof(line) - 1);
> +               string_stream_add(stream, "%s\n", &line[offset]);
> +               total_len += sizeof(line) - offset;
> +       }
> +       num_lines = i;
> +
> +       concat_string = string_stream_get_string(stream);
> +       KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string);
> +       KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len);
> +
> +       /*
> +        * Split the concatenated string at the newlines and check that
> +        * all the original added strings are present.
> +        */
> +       prandom_seed_state(&rnd, 3141592653589793238ULL);
> +       pos = concat_string;
> +       for (i = 0; i < num_lines; ++i) {
> +               string_end = strchr(pos, '\n');
> +               KUNIT_EXPECT_NOT_NULL(test, string_end);
> +
> +               /* Convert to NULL-terminated string */
> +               *string_end = '\0';
> +
> +               offset = prandom_u32_state(&rnd) % (sizeof(line) - 1);
> +               KUNIT_EXPECT_STREQ(test, pos, &line[offset]);
> +
> +               pos = string_end + 1;
> +       }
> +
> +       /* There shouldn't be any more data after this */
> +       KUNIT_EXPECT_EQ(test, strlen(pos), 0);
> +}
> +
> +/* Appending the content of one string stream to another. */
> +static void string_stream_append_test(struct kunit *test)
> +{
> +       static const char * const strings_1[] = {
> +               "one", "two", "three", "four", "five", "six",
> +               "seven", "eight", "nine", "ten",
> +       };
> +       static const char * const strings_2[] = {
> +               "ONE", "TWO", "THREE", "FOUR", "FIVE", "SIZE",
> +               "SEVEN", "EIGHT", "NINE", "TEN",
> +       };

This is fine, but I wonder if it'd be more resilient to potential
changes to the string-string implementation if these arrays didn't
have the same shape (in terms of length and length of substrings).

e.g., if we made the 2nd one "NINE", "EIGHT", "SEVEN"..., so it
doesn't have the same number of strings (due to missing "TEN") and the
lengths of them are not equivalent (strlen("one") != strlen("NINE")).

I doubt it'd make much difference, but maybe it'd catch some nasty
use-after-free or similar errors, and having the strings be different
makes it more obvious that there's not something being tested which
relies on them being the same.

(Don't bother resending the series just for this, though. But if we
have to do a new version anyway...)



> +       struct string_stream *stream_1, *stream_2;
> +       const char *original_content, *stream_2_content;
> +       char *combined_content;
> +       size_t combined_length;
> +       int i;
> +
> +       stream_1 = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
> +
> +       stream_2 = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2);
> +
> +       /* Append content of empty stream to empty stream */
> +       string_stream_append(stream_1, stream_2);
> +       KUNIT_EXPECT_EQ(test, strlen(string_stream_get_string(stream_1)), 0);
> +
> +       /* Add some data to stream_1 */
> +       for (i = 0; i < ARRAY_SIZE(strings_1); ++i)
> +               string_stream_add(stream_1, "%s\n", strings_1[i]);
> +
> +       original_content = string_stream_get_string(stream_1);
> +
> +       /* Append content of empty stream to non-empty stream */
> +       string_stream_append(stream_1, stream_2);
> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream_1), original_content);
> +
> +       /* Add some data to stream_2 */
> +       for (i = 0; i < ARRAY_SIZE(strings_2); ++i)
> +               string_stream_add(stream_2, "%s\n", strings_2[i]);
> +
> +       /* Append content of non-empty stream to non-empty stream */
> +       string_stream_append(stream_1, stream_2);
> +
> +       /*
> +        * End result should be the original content of stream_1 plus
> +        * the content of stream_2.
> +        */
> +       stream_2_content = string_stream_get_string(stream_2);
> +       combined_length = strlen(original_content) + strlen(stream_2_content);
> +       combined_length++; /* for terminating \0 */
> +       combined_content = kunit_kmalloc(test, combined_length, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, combined_content);
> +       snprintf(combined_content, combined_length, "%s%s", original_content, stream_2_content);
> +
> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream_1), combined_content);
> +
> +       /* Append content of non-empty stream to empty stream */
> +       string_stream_destroy(stream_1);
> +
> +       stream_1 = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
> +
> +       string_stream_append(stream_1, stream_2);
> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream_1), stream_2_content);
>  }
>
>  static struct kunit_case string_stream_test_cases[] = {
> -       KUNIT_CASE(string_stream_test_empty_on_creation),
> -       KUNIT_CASE(string_stream_test_not_empty_after_add),
> -       KUNIT_CASE(string_stream_test_get_string),
> +       KUNIT_CASE(string_stream_init_test),
> +       KUNIT_CASE(string_stream_line_add_test),
> +       KUNIT_CASE(string_stream_variable_length_line_test),
> +       KUNIT_CASE(string_stream_append_test),
>         {}
>  };
>
> --
> 2.30.2
>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature