Re: [PATCH v5 02/10] kunit: string-stream: Improve testing of string_stream

[Rae Moar]

On Thu, Aug 24, 2023 at 10:31 AM 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.
>
> Adds string_stream_append_empty_string_test() to test that adding an
> empty string to a string_stream doesn't create a new empty fragment.
>
> Signed-off-by: Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxx>

Hello!

These tests all look good to me. I like all of the details and
comments. Great to see these additions to the string-stream-test!

Reviewed-by: Rae Moar <rmoar@xxxxxxxxxx>

Thanks!
-Rae

> ---
> Changes since V4:
> - Test cases for appending empty strings have been squashed into this
>   patch.
> - Call to string_stream_get_string() is wrapped in a local function
>   get_concatenated_string(). This is to avoid a lot of code churn later
>   when string_stream_get_string() is changed to return an unmanaged buffer.
> ---
>  lib/kunit/string-stream-test.c | 232 ++++++++++++++++++++++++++++++---
>  1 file changed, 216 insertions(+), 16 deletions(-)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 110f3a993250..2b761ba01835 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -11,38 +11,238 @@
>
>  #include "string-stream.h"
>
> -static void string_stream_test_empty_on_creation(struct kunit *test)
> +static char *get_concatenated_string(struct kunit *test, struct string_stream *stream)
>  {
> -       struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL);
> +       char *str = string_stream_get_string(stream);
> +
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, str);
> +
> +       return str;
> +}
> +
> +/* string_stream object is initialized correctly. */
> +static void string_stream_init_test(struct kunit *test)
> +{
> +       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 mentioned in the last version, if this causes a warning we will
look into it on the KUnit side.

>
>         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);
> +
> +               /* 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 = get_concatenated_string(test, 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);
> +       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 = get_concatenated_string(test, 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[] = {
> +               "Apple", "Pear", "Orange", "Banana", "Grape", "Apricot",
> +       };
> +       struct string_stream *stream_1, *stream_2;
> +       const char *original_content, *stream_2_content;

I would maybe consider changing the name original_content to
stream_1_content but definitely not worth it as this version looks
very good.

> +       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(get_concatenated_string(test, 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 = get_concatenated_string(test, stream_1);
> +
> +       /* Append content of empty stream to non-empty stream */
> +       string_stream_append(stream_1, stream_2);
> +       KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, 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 = get_concatenated_string(test, 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, get_concatenated_string(test, 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, get_concatenated_string(test, stream_1), stream_2_content);
> +}
> +
> +/* Adding an empty string should not create a fragment. */
> +static void string_stream_append_empty_string_test(struct kunit *test)
> +{
> +       struct string_stream *stream;
> +       int original_frag_count;
> +
> +       stream = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
> +
> +       /* Formatted empty string */
> +       string_stream_add(stream, "%s", "");
> +       KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
> +       KUNIT_EXPECT_TRUE(test, list_empty(&stream->fragments));
> +
> +       /* Adding an empty string to a non-empty stream */
> +       string_stream_add(stream, "Add this line");
> +       original_frag_count = list_count_nodes(&stream->fragments);
> +
> +       string_stream_add(stream, "%s", "");
> +       KUNIT_EXPECT_EQ(test, list_count_nodes(&stream->fragments), original_frag_count);
> +       KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream), "Add this line");
>  }
>
>  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),
> +       KUNIT_CASE(string_stream_append_empty_string_test),
>         {}
>  };
>
> --
> 2.30.2
>