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>
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@google.com --- lib/kunit/assert.c | 39 +++++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-)
diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c index 33acdaa28a7d..e0ec7d6fed6f 100644 --- a/lib/kunit/assert.c +++ b/lib/kunit/assert.c @@ -85,6 +85,29 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, } EXPORT_SYMBOL_GPL(kunit_ptr_not_err_assert_format);
+/* Checks if `text` is a literal representing `value`, e.g. "5" and 5 */ +static bool is_literal(struct kunit *test, const char *text, long long value, + gfp_t gfp) +{ + char *buffer; + int len; + bool ret; + + len = snprintf(NULL, 0, "%lld", value); + if (strlen(text) != len) + return false; + + buffer = kunit_kmalloc(test, len+1, gfp); + if (!buffer) + return false; + + snprintf(buffer, len+1, "%lld", value); + ret = strncmp(buffer, text, len) == 0; + + kunit_kfree(test, buffer); + return ret; +} + void kunit_binary_assert_format(const struct kunit_assert *assert, struct string_stream *stream) { @@ -97,12 +120,16 @@ void kunit_binary_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 == %lld\n", - binary_assert->left_text, - binary_assert->left_value); - string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld", - binary_assert->right_text, - binary_assert->right_value); + if (!is_literal(stream->test, binary_assert->left_text, + binary_assert->left_value, stream->gfp)) + string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld\n", + binary_assert->left_text, + binary_assert->left_value); + if (!is_literal(stream->test, binary_assert->right_text, + binary_assert->right_value, stream->gfp)) + string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld", + binary_assert->right_text, + binary_assert->right_value); kunit_assert_print_msg(assert, stream); } EXPORT_SYMBOL_GPL(kunit_binary_assert_format);
base-commit: e5ff2cb9cf67a542f2ec7fb87e24934c88b32678
On Fri, Jan 29, 2021 at 10:26 AM Daniel Latypov dlatypov@google.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.)
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.
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.)
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@google.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.
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.)
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
Either way, though, this is:
Tested-by: David Gow davidgow@google.com
Cheers, -- David
On Thu, Jan 28, 2021 at 8:51 PM David Gow davidgow@google.com wrote:
On Fri, Jan 29, 2021 at 10:26 AM Daniel Latypov dlatypov@google.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@google.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@google.com
Cheers, -- David
On Thu, Jan 28, 2021 at 6:26 PM Daniel Latypov dlatypov@google.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>
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@google.com
I don't feel very strongly about this either way. In any case:
Reviewed-by: Brendan Higgins brendanhiggins@google.com
linux-kselftest-mirror@lists.linaro.org
-
Brendan Higgins -
Daniel Latypov -
David Gow