| Message ID | 20210205221808.1966010-1-dlatypov@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Shuah Khan |
| Headers | show |
| Series | kunit: make KUNIT_EXPECT_STREQ() quote values, don't print literals | expand |
On Fri, Feb 5, 2021 at 2:18 PM Daniel Latypov <dlatypov@google.com> wrote: > > Before: > > Expected str == "world", but > > str == hello > > "world" == world > > After: > > Expected str == "world", but > > str == "hello" > <we don't need to tell the user that "world" == "world"> > > Note: like the literal ellision for integers, this doesn't handle the > case of > KUNIT_EXPECT_STREQ(test, "hello", "world") > since we don't expect it to realistically happen in checked in tests. > (If you really wanted a test to fail, KUNIT_FAIL("msg") exists) > > In that case, you'd get: > > Expected "hello" == "world", but > <output for next failure> > > Signed-off-by: Daniel Latypov <dlatypov@google.com> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
On 4/2/21 3:35 AM, Brendan Higgins wrote: > On Fri, Feb 5, 2021 at 2:18 PM Daniel Latypov <dlatypov@google.com> wrote: >> >> Before: >>> Expected str == "world", but >>> str == hello >>> "world" == world >> >> After: >>> Expected str == "world", but >>> str == "hello" >> <we don't need to tell the user that "world" == "world"> >> >> Note: like the literal ellision for integers, this doesn't handle the >> case of >> KUNIT_EXPECT_STREQ(test, "hello", "world") >> since we don't expect it to realistically happen in checked in tests. >> (If you really wanted a test to fail, KUNIT_FAIL("msg") exists) >> >> In that case, you'd get: >>> Expected "hello" == "world", but >> <output for next failure> >> >> Signed-off-by: Daniel Latypov <dlatypov@google.com> > > Reviewed-by: Brendan Higgins <brendanhiggins@google.com> > Hi Daniel, Please run checkpatch on your patches in the future. I am seeing a few checkpatch readability type improvements that can be made. Please make changes and send v2 with Brendan's Reviewed-by. thanks, -- Shuah
On Fri, Apr 2, 2021 at 10:47 AM Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 4/2/21 3:35 AM, Brendan Higgins wrote: > > On Fri, Feb 5, 2021 at 2:18 PM Daniel Latypov <dlatypov@google.com> wrote: > >> > >> Before: > >>> Expected str == "world", but > >>> str == hello > >>> "world" == world > >> > >> After: > >>> Expected str == "world", but > >>> str == "hello" > >> <we don't need to tell the user that "world" == "world"> > >> > >> Note: like the literal ellision for integers, this doesn't handle the > >> case of > >> KUNIT_EXPECT_STREQ(test, "hello", "world") > >> since we don't expect it to realistically happen in checked in tests. > >> (If you really wanted a test to fail, KUNIT_FAIL("msg") exists) > >> > >> In that case, you'd get: > >>> Expected "hello" == "world", but > >> <output for next failure> > >> > >> Signed-off-by: Daniel Latypov <dlatypov@google.com> > > > > Reviewed-by: Brendan Higgins <brendanhiggins@google.com> > > > > Hi Daniel, > > Please run checkpatch on your patches in the future. I am seeing > a few checkpatch readability type improvements that can be made. > > Please make changes and send v2 with Brendan's Reviewed-by. Are there some flags you'd like me to pass to checkpatch? $ ./scripts/checkpatch.pl --git HEAD total: 0 errors, 0 warnings, 42 lines checked Commit f66884e8b831 ("kunit: make KUNIT_EXPECT_STREQ() quote values, don't print literals") has no obvious style problems and is ready for submission. I just rebased onto linus/master again since I know checkpatch.pl's default behavior had changed recently, but I didn't see any errors there. I know this commit made some lines go just over 80 characters, so $ ./scripts/checkpatch.pl --max-line-length=80 --git HEAD ... total: 0 errors, 4 warnings, 42 lines checked I can go and line wrap these but had figured they were more readable this way if checkpatch.pl no longer complained by default. Thanks, Daniel > > thanks, > -- Shuah
On 4/2/21 1:09 PM, Daniel Latypov wrote: > On Fri, Apr 2, 2021 at 10:47 AM Shuah Khan <skhan@linuxfoundation.org> wrote: >> >> On 4/2/21 3:35 AM, Brendan Higgins wrote: >>> On Fri, Feb 5, 2021 at 2:18 PM Daniel Latypov <dlatypov@google.com> wrote: >>>> >>>> Before: >>>>> Expected str == "world", but >>>>> str == hello >>>>> "world" == world >>>> >>>> After: >>>>> Expected str == "world", but >>>>> str == "hello" >>>> <we don't need to tell the user that "world" == "world"> >>>> >>>> Note: like the literal ellision for integers, this doesn't handle the >>>> case of >>>> KUNIT_EXPECT_STREQ(test, "hello", "world") >>>> since we don't expect it to realistically happen in checked in tests. >>>> (If you really wanted a test to fail, KUNIT_FAIL("msg") exists) >>>> >>>> In that case, you'd get: >>>>> Expected "hello" == "world", but >>>> <output for next failure> >>>> >>>> Signed-off-by: Daniel Latypov <dlatypov@google.com> >>> >>> Reviewed-by: Brendan Higgins <brendanhiggins@google.com> >>> >> >> Hi Daniel, >> >> Please run checkpatch on your patches in the future. I am seeing >> a few checkpatch readability type improvements that can be made. >> >> Please make changes and send v2 with Brendan's Reviewed-by. > > Are there some flags you'd like me to pass to checkpatch? > > $ ./scripts/checkpatch.pl --git HEAD > total: 0 errors, 0 warnings, 42 lines checked > My commit script uses --strict which shows readability errors. > Commit f66884e8b831 ("kunit: make KUNIT_EXPECT_STREQ() quote values, > don't print literals") has no obvious style problems and is ready for > submission. > > I just rebased onto linus/master again since I know checkpatch.pl's > default behavior had changed recently, but I didn't see any errors > there. > > I know this commit made some lines go just over 80 characters, so > $ ./scripts/checkpatch.pl --max-line-length=80 --git HEAD > ... > total: 0 errors, 4 warnings, 42 lines checked > Don't worry about line wrap warns. I just ignore them. :) thanks, -- Shuah
On Fri, Apr 2, 2021 at 12:19 PM Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 4/2/21 1:09 PM, Daniel Latypov wrote: > > On Fri, Apr 2, 2021 at 10:47 AM Shuah Khan <skhan@linuxfoundation.org> wrote: > >> > >> On 4/2/21 3:35 AM, Brendan Higgins wrote: > >>> On Fri, Feb 5, 2021 at 2:18 PM Daniel Latypov <dlatypov@google.com> wrote: > >>>> > >>>> Before: > >>>>> Expected str == "world", but > >>>>> str == hello > >>>>> "world" == world > >>>> > >>>> After: > >>>>> Expected str == "world", but > >>>>> str == "hello" > >>>> <we don't need to tell the user that "world" == "world"> > >>>> > >>>> Note: like the literal ellision for integers, this doesn't handle the > >>>> case of > >>>> KUNIT_EXPECT_STREQ(test, "hello", "world") > >>>> since we don't expect it to realistically happen in checked in tests. > >>>> (If you really wanted a test to fail, KUNIT_FAIL("msg") exists) > >>>> > >>>> In that case, you'd get: > >>>>> Expected "hello" == "world", but > >>>> <output for next failure> > >>>> > >>>> Signed-off-by: Daniel Latypov <dlatypov@google.com> > >>> > >>> Reviewed-by: Brendan Higgins <brendanhiggins@google.com> > >>> > >> > >> Hi Daniel, > >> > >> Please run checkpatch on your patches in the future. I am seeing > >> a few checkpatch readability type improvements that can be made. > >> > >> Please make changes and send v2 with Brendan's Reviewed-by. > > > > Are there some flags you'd like me to pass to checkpatch? > > > > $ ./scripts/checkpatch.pl --git HEAD > > total: 0 errors, 0 warnings, 42 lines checked > > > > My commit script uses --strict which shows readability errors. Oh neat, TIL. I'll make sure to use that in the future, thanks! v2: https://lore.kernel.org/linux-kselftest/20210402193357.819176-1-dlatypov@google.com/ > > > Commit f66884e8b831 ("kunit: make KUNIT_EXPECT_STREQ() quote values, > > don't print literals") has no obvious style problems and is ready for > > submission. > > > > I just rebased onto linus/master again since I know checkpatch.pl's > > default behavior had changed recently, but I didn't see any errors > > there. > > > > I know this commit made some lines go just over 80 characters, so > > $ ./scripts/checkpatch.pl --max-line-length=80 --git HEAD > > ... > > total: 0 errors, 4 warnings, 42 lines checked > > > > Don't worry about line wrap warns. I just ignore them. :) > > thanks, > -- Shuah > > >
diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c index e0ec7d6fed6f..176ef547fa94 100644 --- a/lib/kunit/assert.c +++ b/lib/kunit/assert.c @@ -156,6 +156,22 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert, } EXPORT_SYMBOL_GPL(kunit_binary_ptr_assert_format); +/* Checks if KUNIT_EXPECT_STREQ() args were string literals. + * Note: `text` will have ""s where as `value` will not. + */ +static bool is_str_literal(const char *text, const char *value) +{ + int len; + + len = strlen(text); + if (len < 2) + return false; + if (text[0] != '\"' || text[len-1] != '\"') + return false; + + return strncmp(text+1, value, len-2) == 0; +} + void kunit_binary_str_assert_format(const struct kunit_assert *assert, struct string_stream *stream) { @@ -168,12 +184,14 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert, binary_assert->left_text, binary_assert->operation, binary_assert->right_text); - string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %s\n", - binary_assert->left_text, - binary_assert->left_value); - string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %s", - binary_assert->right_text, - binary_assert->right_value); + if (!is_str_literal(binary_assert->left_text, binary_assert->left_value)) + string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == \"%s\"\n", + binary_assert->left_text, + binary_assert->left_value); + if (!is_str_literal(binary_assert->right_text, binary_assert->right_value)) + string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == \"%s\"", + binary_assert->right_text, + binary_assert->right_value); kunit_assert_print_msg(assert, stream); } EXPORT_SYMBOL_GPL(kunit_binary_str_assert_format);