Message-ID: <CAGS_qxorPoK=dyZs_SyNOv1_Z0RjJrn97A03sbyF0UmsopcdLQ@mail.gmail.com>
Date:   Fri, 29 Jan 2021 10:19:56 -0800
From:   Daniel Latypov <dlatypov@...gle.com>
To:     David Gow <davidgow@...gle.com>
Cc:     Brendan Higgins <brendanhiggins@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH] kunit: don't show `1 == 1` in failed assertion messages

On Thu, Jan 28, 2021 at 8:51 PM David Gow <davidgow@...gle.com> wrote:
>
> On Fri, Jan 29, 2021 at 10:26 AM Daniel Latypov <dlatypov@...gle.com> wrote:
> >
> > Currently, given something (fairly dystopian) like
> > > KUNIT_EXPECT_EQ(test, 2 + 2, 5)
> >
> > KUnit will prints a failure message like this.
> > >  Expected 2 + 2 == 5, but
> > >      2 + 2 == 4
> > >      5 == 5
> >
> > With this patch, the output just becomes
> > >  Expected 2 + 2 == 5, but
> > >      2 + 2 == 4
> >
> > This patch is slightly hacky, but it's quite common* to compare an
> > expression to a literal integer value, so this can make KUnit less
> > chatty in many cases. (This patch also fixes variants like
> > KUNIT_EXPECT_GT, LE, et al.).
> >
> > It also allocates an additional string briefly, but given this only
> > happens on test failures, it doesn't seem too bad a tradeoff.
> > Also, in most cases it'll realize the lengths are unequal and bail out
> > before the allocation.
> >
> > We could save the result of the formatted string to avoid wasting this
> > extra work, but it felt cleaner to leave it as-is.
> >
> > Edge case: for something silly and unrealistic like
> > > KUNIT_EXPECT_EQ(test, 4, 5);
> >
> > It'll generate this message with a trailing "but"
> > >  Expected 2 + 2 == 5, but
> > >  <next line of normal output>
>
> I assume this is supposed to say "Expected 4 == 5" here.
> (I tested it to make sure, and that's what it did here.)

Ah yes, too much copy-paste.

>
> Personally, I'd ideally like to get rid of the ", but", or even add a
> "but 4 != 5" style second line. Particularly in case the next line in
> the output might be confused for the rest of a sentence.

Given the apparent interest in other types (STR_EQ) of literal
ellision, maybe this should be done.
But I'd be tempted to have that change come later once at least the
str_eq version is in place.

>
> That being said, this is a pretty silly edge case: I'd be worried if
> we ever saw that case in an actual submitted test. People might see it
> a bit while debugging, though: particularly if they're using
> KUNIT_EXPECT_EQ(test, 1, 2) as a way of forcing a test to fail. (I've
> done this while testing tooling, for instance.)

Same/Agreed on all points.

>
> >
> > It didn't feel worth adding a check up-front to see if both sides are
> > literals to handle this better.
> >
> > *A quick grep suggests 100+ comparisons to an integer literal as the
> > right hand side.
> >
> > Signed-off-by: Daniel Latypov <dlatypov@...gle.com>
> > ---
>
> I tested this, and it works well: the results are definitely more
> human readable. I could see it making things slightly more complicated
> for people who wanted to automatically parse assertion errors, but
> no-one is doing that, and the extra complexity is pretty minimal
> anyway.

Hmm, machine parsing of the contents of failures is interesting.
But in general, that feels that requires a more structured format.

I hate to invoke it, but the tooling I've seen that's parsed the
"expected" and "actual" values has represented them as XML elements.

>
> One thing which might be worth doing is expanding this to
> KUNIT_EXPECT_STREQ() and/or KUNIT_EXPECT_PTR_EQ(). These have slightly
> more complicated formatting (quotes, leading 0s, etc), though.
> Comparing pointer literals is pretty unlikely to show up, though, so I
> don't think it's as important. (I thought that maybe the KASAN shadow
> memory tests might use them, but a quick look didn't reveal any.)
>

Ack. Actually, the string literal check was smaller, see below.
I debated sending a patch out for that, but this case mattered more
and I wasn't sure if it would be acceptable or not.
It felt it would be incongruous to only handle strings and not the
much more common integer case.

So if the hackier, more costly integer comparison seems fine, I might
actually go and send out the str patch that I already have sitting
around anyways.

+/* 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;
+}
+

This produces
[10:05:59]     Expected str == "world", but
[10:05:59]         str == hello

One misgiving I had was whether we should "fix" the string printing to
quote the values or not before adding `is_str_literal()` in.
Having just "str == hello" where neither is quoted is a bit unclear
and the extra "world == world" line sorta helped make that more clear,
ha.

David, I can send a version of this patch w/ a fixed commit message
and then tack on the str changes as children.
Would you prefer that?

> For the record, this is what STREQ()/PTR_EQ()/ failures with literals look like:
> # example_simple_test: EXPECTATION FAILED at lib/kunit/kunit-example-test.c:31
> Expected "abc" == "abd", but
>     "abc" == abc
>     "abd" == abd
> # example_simple_test: EXPECTATION FAILED at lib/kunit/kunit-example-test.c:33
> Expected 0x124 == 0x1234, but
>     0x124 == 0000000000000124
>     0x1234 == 0000000000001234

Yeah, I had considered PTR_EQ(), but it seemed more complex and also
less likely to show up.
And outside of very niche use cases (which probably don't work too on
UML, tbh...), it felt like an anti-pattern to have hard-coded pointers
in unit tests.

>
> Either way, though, this is:
>
> Tested-by: David Gow <davidgow@...gle.com>
>
> Cheers,
> -- David

Powered by blists - more mailing lists