| Message ID | 20230814132309.32641-1-rf@opensource.cirrus.com |
|---|---|
| Headers | show |
| Series | kunit: Add dynamically-extending log | expand |
On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > > 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@opensource.cirrus.com> > --- Looks good. One minor note below (not worth resending the series to fix by itself, though). If you wanted to combine this with the previous patch, that'd be fine too. (I don't mind either way.) Reviewed-by: David Gow <davidgow@google.com> Cheers, -- David > lib/kunit/string-stream-test.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c > index 1d46d5f06d2a..efe13e3322b5 100644 > --- a/lib/kunit/string-stream-test.c > +++ b/lib/kunit/string-stream-test.c > @@ -206,11 +206,32 @@ static void string_stream_append_test(struct kunit *test) > KUNIT_EXPECT_STREQ(test, string_stream_get_string(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; > + > + 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"); > + string_stream_add(stream, "%s", ""); > + KUNIT_EXPECT_EQ(test, list_count_nodes(&stream->fragments), 1); While this is fine, I do wonder whether the more future-proof thing to do would be to check that the number of fragments hasn't changed after adding the empty string, rather than that it's definitely 1. (In practice, even with a fancier allocation strategy, I can't imagine us splitting "Add this line" into multiple fragments, but I think it's slightly clearer that what we're testing is that the empty string doesn't increase it.) > + KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream), "Add this line"); > +} > + > static struct kunit_case string_stream_test_cases[] = { > 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 >
On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > > Add test cases for testing the string_stream feature that appends a > newline to strings that do not already end with a newline. > > string_stream_no_auto_newline_test() tests with this feature disabled. > Newlines should not be added or dropped. > > string_stream_auto_newline_test() tests with this feature enabled. > Newlines should be added to lines that do not end with a newline. > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> > --- Looks good to me. I wouldn't mind if you added the extra test strings to the non-newline case, but can do without if you feel it's excessive. Reviewed-by: David Gow <davidgow@google.com> -- David > lib/kunit/string-stream-test.c | 51 ++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c > index efe13e3322b5..46c2ac162fe8 100644 > --- a/lib/kunit/string-stream-test.c > +++ b/lib/kunit/string-stream-test.c > @@ -23,6 +23,7 @@ static void string_stream_init_test(struct kunit *test) > KUNIT_EXPECT_TRUE(test, list_empty(&stream->fragments)); > KUNIT_EXPECT_PTR_EQ(test, stream->test, test); > KUNIT_EXPECT_EQ(test, stream->gfp, GFP_KERNEL); > + KUNIT_EXPECT_FALSE(test, stream->append_newlines); > > KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream)); > } > @@ -226,12 +227,62 @@ static void string_stream_append_empty_string_test(struct kunit *test) > KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream), "Add this line"); > } > > +/* Adding strings without automatic newline appending */ > +static void string_stream_no_auto_newline_test(struct kunit *test) > +{ > + struct string_stream *stream; > + > + stream = alloc_string_stream(test, GFP_KERNEL); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream); > + > + /* > + * Add some strings with and without newlines. All formatted > + * newlines should be preserved. No extra newlines should be > + * added. > + */ > + string_stream_add(stream, "One"); > + string_stream_add(stream, "Two\n"); > + string_stream_add(stream, "%s\n", "Three"); > + string_stream_add(stream, "Four"); > + KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream), > + "OneTwo\nThree\nFour"); > +} > + > +/* Adding strings with automatic newline appending */ > +static void string_stream_auto_newline_test(struct kunit *test) > +{ > + struct string_stream *stream; > + > + stream = alloc_string_stream(test, GFP_KERNEL); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream); > + > + string_stream_set_append_newlines(stream, true); > + KUNIT_EXPECT_TRUE(test, stream->append_newlines); > + > + /* > + * Add some strings with and without newlines. Newlines should > + * be appended to lines that do not end with \n, but newlines > + * resulting from the formatting should not be changed. > + */ > + string_stream_add(stream, "One"); > + string_stream_add(stream, "Two\n"); > + string_stream_add(stream, "%s\n", "Three"); > + string_stream_add(stream, "%s", "Four\n"); > + string_stream_add(stream, "Five\n%s", "Six"); > + string_stream_add(stream, "Seven\n\n"); > + string_stream_add(stream, "Eight"); > + KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream), > + "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n"); > +} > + > static struct kunit_case string_stream_test_cases[] = { > 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), > + KUNIT_CASE(string_stream_no_auto_newline_test), > + KUNIT_CASE(string_stream_auto_newline_test), > {} > }; > > -- > 2.30.2 >
On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > > This patch chain changes the logging implementation to use string_stream > so that the log will grow dynamically. > > The first 8 patches add test code for string_stream, and make some > changes to string_stream needed to be able to use it for the log. > > The final patch adds a performance report of string_stream. > > CHANGES SINCE V3: > > Completely rewritten to use string_stream instead of implementing a > separate extending-buffer implementation for logging. > > I have used the performance test from the final patch on my original > fixed-size-fragment implementation from V3 to get a comparison of the > two implementations (run on i3-8145UE CPU @ 2.20GHz): > > string_stream V3 fixed-size-buffer > Time elapsed: 7748 us 3251 us > Total string length: 573890 573890 > Bytes requested: 823994 728336 > Actual bytes allocated: 1061440 728352 > > I don't think the difference is enough to be worth complicating the > string_stream implementation with my fixed-fragment implementation from > V3 of this patch chain. > Thanks very much! I think this is good to go: I've added a few small comments on various patches, but none of them are serious enough to make me feel we _need_ a new version. I'll leave it for a day or two in case there are any other comments or any changes you want to make, otherwise this can be merged. I agree the performance difference isn't worth the effort. If we change our minds and want to change the implementation over later, that shouldn't be a problem either. So let's stick with it as is. Cheers, -- David > Richard Fitzgerald (10): > kunit: string-stream: Improve testing of string_stream > kunit: string-stream: Don't create a fragment for empty strings > kunit: string-stream: Add cases for adding empty strings to a > string_stream > kunit: string-stream: Add option to make all lines end with newline > kunit: string-stream: Add cases for string_stream newline appending > kunit: string-stream: Pass struct kunit to string_stream_get_string() > kunit: string-stream: Decouple string_stream from kunit > kunit: string-stream: Add test for freeing resource-managed > string_stream > kunit: Use string_stream for test log > kunit: string-stream: Test performance of string_stream > > include/kunit/test.h | 14 +- > lib/kunit/Makefile | 5 +- > lib/kunit/debugfs.c | 36 ++- > lib/kunit/kunit-test.c | 52 +--- > lib/kunit/log-test.c | 72 ++++++ > lib/kunit/string-stream-test.c | 447 +++++++++++++++++++++++++++++++-- > lib/kunit/string-stream.c | 129 +++++++--- > lib/kunit/string-stream.h | 22 +- > lib/kunit/test.c | 48 +--- > 9 files changed, 656 insertions(+), 169 deletions(-) > create mode 100644 lib/kunit/log-test.c > > -- > 2.30.2 >
On Mon, Aug 14, 2023 at 9:23 AM Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > > This patch chain changes the logging implementation to use string_stream > so that the log will grow dynamically. > > The first 8 patches add test code for string_stream, and make some > changes to string_stream needed to be able to use it for the log. > > The final patch adds a performance report of string_stream. > > CHANGES SINCE V3: > > Completely rewritten to use string_stream instead of implementing a > separate extending-buffer implementation for logging. > > I have used the performance test from the final patch on my original > fixed-size-fragment implementation from V3 to get a comparison of the > two implementations (run on i3-8145UE CPU @ 2.20GHz): > > string_stream V3 fixed-size-buffer > Time elapsed: 7748 us 3251 us > Total string length: 573890 573890 > Bytes requested: 823994 728336 > Actual bytes allocated: 1061440 728352 > > I don't think the difference is enough to be worth complicating the > string_stream implementation with my fixed-fragment implementation from > V3 of this patch chain. Hello! I just wanted to note that I have tested this series and it looks good to me. I specifically looked over the newline handling and that looks really good. I understand you will likely be doing a new version of this. But other than the things noted in comments by David, this seems to be working really well. Tested-by: Rae Moar <rmoar@google.com> Thanks for all the work you did in moving this framework to string-stream! -Rae > > Richard Fitzgerald (10): > kunit: string-stream: Improve testing of string_stream > kunit: string-stream: Don't create a fragment for empty strings > kunit: string-stream: Add cases for adding empty strings to a > string_stream > kunit: string-stream: Add option to make all lines end with newline > kunit: string-stream: Add cases for string_stream newline appending > kunit: string-stream: Pass struct kunit to string_stream_get_string() > kunit: string-stream: Decouple string_stream from kunit > kunit: string-stream: Add test for freeing resource-managed > string_stream > kunit: Use string_stream for test log > kunit: string-stream: Test performance of string_stream > > include/kunit/test.h | 14 +- > lib/kunit/Makefile | 5 +- > lib/kunit/debugfs.c | 36 ++- > lib/kunit/kunit-test.c | 52 +--- > lib/kunit/log-test.c | 72 ++++++ > lib/kunit/string-stream-test.c | 447 +++++++++++++++++++++++++++++++-- > lib/kunit/string-stream.c | 129 +++++++--- > lib/kunit/string-stream.h | 22 +- > lib/kunit/test.c | 48 +--- > 9 files changed, 656 insertions(+), 169 deletions(-) > create mode 100644 lib/kunit/log-test.c > > -- > 2.30.2 >
This patch chain changes the logging implementation to use string_stream so that the log will grow dynamically. The first 8 patches add test code for string_stream, and make some changes to string_stream needed to be able to use it for the log. The final patch adds a performance report of string_stream. CHANGES SINCE V3: Completely rewritten to use string_stream instead of implementing a separate extending-buffer implementation for logging. I have used the performance test from the final patch on my original fixed-size-fragment implementation from V3 to get a comparison of the two implementations (run on i3-8145UE CPU @ 2.20GHz): string_stream V3 fixed-size-buffer Time elapsed: 7748 us 3251 us Total string length: 573890 573890 Bytes requested: 823994 728336 Actual bytes allocated: 1061440 728352 I don't think the difference is enough to be worth complicating the string_stream implementation with my fixed-fragment implementation from V3 of this patch chain. Richard Fitzgerald (10): kunit: string-stream: Improve testing of string_stream kunit: string-stream: Don't create a fragment for empty strings kunit: string-stream: Add cases for adding empty strings to a string_stream kunit: string-stream: Add option to make all lines end with newline kunit: string-stream: Add cases for string_stream newline appending kunit: string-stream: Pass struct kunit to string_stream_get_string() kunit: string-stream: Decouple string_stream from kunit kunit: string-stream: Add test for freeing resource-managed string_stream kunit: Use string_stream for test log kunit: string-stream: Test performance of string_stream include/kunit/test.h | 14 +- lib/kunit/Makefile | 5 +- lib/kunit/debugfs.c | 36 ++- lib/kunit/kunit-test.c | 52 +--- lib/kunit/log-test.c | 72 ++++++ lib/kunit/string-stream-test.c | 447 +++++++++++++++++++++++++++++++-- lib/kunit/string-stream.c | 129 +++++++--- lib/kunit/string-stream.h | 22 +- lib/kunit/test.c | 48 +--- 9 files changed, 656 insertions(+), 169 deletions(-) create mode 100644 lib/kunit/log-test.c