diff mbox series

[v2,6/7] kunit: Print last test location on fault

Message ID 20240301194037.532117-7-mic@digikod.net (mailing list archive)
State Superseded
Headers show
Series Handle faults in KUnit tests | expand

Commit Message

Mickaël Salaün March 1, 2024, 7:40 p.m. UTC
This helps identify the location of test faults.

Cc: Brendan Higgins <brendanhiggins@google.com>
Cc: David Gow <davidgow@google.com>
Cc: Rae Moar <rmoar@google.com>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20240301194037.532117-7-mic@digikod.net
---

Changes since v1:
* Added Kees's Reviewed-by.
---
 include/kunit/test.h  | 24 +++++++++++++++++++++---
 lib/kunit/try-catch.c | 10 +++++++---
 2 files changed, 28 insertions(+), 6 deletions(-)

Comments

David Gow March 12, 2024, 4:54 a.m. UTC | #1
On Sat, 2 Mar 2024 at 03:40, Mickaël Salaün <mic@digikod.net> wrote:
>
> This helps identify the location of test faults.
>
> Cc: Brendan Higgins <brendanhiggins@google.com>
> Cc: David Gow <davidgow@google.com>
> Cc: Rae Moar <rmoar@google.com>
> Cc: Shuah Khan <skhan@linuxfoundation.org>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20240301194037.532117-7-mic@digikod.net
> ---

I like the idea of this, but am a little bit worried about how
confusing it might be, given that the location only updates on those
particular macros.

Maybe the answer is to make the __KUNIT_SAVE_LOC() macro, or something
equivalent, a supported API.

One possibility would be to have a KUNIT_MARKER() macro. If we really
wanted to, we could expand it to take a string so we can have a more
user-friendly KUNIT_MARKER(test, "parsing packet") description of
where things went wrong. Another could be to extend this to use the
code tagging framework[1], if that lands.

That being said, I think this is still an improvement without any of
those features. I've left a few comments below. Let me know what you
think.

Cheers,
-- David

[1]: https://lwn.net/Articles/906660/
>
> Changes since v1:
> * Added Kees's Reviewed-by.
> ---
>  include/kunit/test.h  | 24 +++++++++++++++++++++---
>  lib/kunit/try-catch.c | 10 +++++++---
>  2 files changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index fcb4a4940ace..f3aa66eb0087 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -301,6 +301,8 @@ struct kunit {
>         struct list_head resources; /* Protected by lock. */
>
>         char status_comment[KUNIT_STATUS_COMMENT_SIZE];
> +       /* Saves the last seen test. Useful to help with faults. */
> +       struct kunit_loc last_seen;
>  };
>
>  static inline void kunit_set_failure(struct kunit *test)
> @@ -567,6 +569,15 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt,
>  #define kunit_err(test, fmt, ...) \
>         kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
>
> +/*
> + * Must be called at the beginning of each KUNIT_*_ASSERTION().
> + * Cf. KUNIT_CURRENT_LOC.
> + */
> +#define _KUNIT_SAVE_LOC(test) do {                                            \
> +       WRITE_ONCE(test->last_seen.file, __FILE__);                            \
> +       WRITE_ONCE(test->last_seen.line, __LINE__);                            \
> +} while (0)

Can we get rid of the leading '_', make this public, and document it?
If we want to rename it to KUNIT_MARKER() or similar, that might work
better, too.

> +
>  /**
>   * KUNIT_SUCCEED() - A no-op expectation. Only exists for code clarity.
>   * @test: The test context object.
> @@ -575,7 +586,7 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt,
>   * words, it does nothing and only exists for code clarity. See
>   * KUNIT_EXPECT_TRUE() for more information.
>   */
> -#define KUNIT_SUCCEED(test) do {} while (0)
> +#define KUNIT_SUCCEED(test) _KUNIT_SAVE_LOC(test)
>
>  void __noreturn __kunit_abort(struct kunit *test);
>
> @@ -601,14 +612,16 @@ void __kunit_do_failed_assertion(struct kunit *test,
>  } while (0)
>
>
> -#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...)                     \
> +#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) do {                \
> +       _KUNIT_SAVE_LOC(test);                                                 \
>         _KUNIT_FAILED(test,                                                    \
>                       assert_type,                                             \
>                       kunit_fail_assert,                                       \
>                       kunit_fail_assert_format,                                \
>                       {},                                                      \
>                       fmt,                                                     \
> -                     ##__VA_ARGS__)
> +                     ##__VA_ARGS__);                                          \
> +} while (0)
>
>  /**
>   * KUNIT_FAIL() - Always causes a test to fail when evaluated.
> @@ -637,6 +650,7 @@ void __kunit_do_failed_assertion(struct kunit *test,
>                               fmt,                                             \
>                               ...)                                             \
>  do {                                                                          \
> +       _KUNIT_SAVE_LOC(test);                                                 \
>         if (likely(!!(condition_) == !!expected_true_))                        \
>                 break;                                                         \
>                                                                                \
> @@ -698,6 +712,7 @@ do {                                                                               \
>                 .right_text = #right,                                          \
>         };                                                                     \
>                                                                                \
> +       _KUNIT_SAVE_LOC(test);                                                 \
>         if (likely(__left op __right))                                         \
>                 break;                                                         \
>                                                                                \
> @@ -758,6 +773,7 @@ do {                                                                               \
>                 .right_text = #right,                                          \
>         };                                                                     \
>                                                                                \
> +       _KUNIT_SAVE_LOC(test);                                                 \
>         if (likely((__left) && (__right) && (strcmp(__left, __right) op 0)))   \
>                 break;                                                         \
>                                                                                \
> @@ -791,6 +807,7 @@ do {                                                                               \
>                 .right_text = #right,                                          \
>         };                                                                     \
>                                                                                \
> +       _KUNIT_SAVE_LOC(test);                                                 \
>         if (likely(__left && __right))                                         \
>                 if (likely(memcmp(__left, __right, __size) op 0))              \
>                         break;                                                 \
> @@ -815,6 +832,7 @@ do {                                                                               \
>  do {                                                                          \
>         const typeof(ptr) __ptr = (ptr);                                       \
>                                                                                \
> +       _KUNIT_SAVE_LOC(test);                                                 \
>         if (!IS_ERR_OR_NULL(__ptr))                                            \
>                 break;                                                         \
>                                                                                \
> diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> index c6ee4db0b3bd..2ec21c6918f3 100644
> --- a/lib/kunit/try-catch.c
> +++ b/lib/kunit/try-catch.c
> @@ -91,9 +91,13 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context)
>
>         if (exit_code == -EFAULT)
>                 try_catch->try_result = 0;
> -       else if (exit_code == -EINTR)
> -               kunit_err(test, "try faulted\n");
> -       else if (exit_code == -ETIMEDOUT)
> +       else if (exit_code == -EINTR) {
> +               if (test->last_seen.file)
> +                       kunit_err(test, "try faulted after %s:%d\n",
> +                                 test->last_seen.file, test->last_seen.line);

It's possibly a bit confusing to just say "after file:line",
particularly if we then loop or call a function "higher up" in the
file. Maybe something like "try faulted: last line seen %s:%d" is
clearer.

> +               else
> +                       kunit_err(test, "try faulted before the first test\n");

I don't like using "test" here, as it introduces ambiguity between
"kunit tests" and "assertions/expectations" if we call them both
tests. Maybe just "try faulted" here, or "try faulted (no markers
seen)" or similar?


> +       } else if (exit_code == -ETIMEDOUT)
>                 kunit_err(test, "try timed out\n");
>         else if (exit_code)
>                 kunit_err(test, "Unknown error: %d\n", exit_code);
> --
> 2.44.0
>
Mickaël Salaün March 12, 2024, 12:15 p.m. UTC | #2
On Tue, Mar 12, 2024 at 12:54:48PM +0800, David Gow wrote:
> On Sat, 2 Mar 2024 at 03:40, Mickaël Salaün <mic@digikod.net> wrote:
> >
> > This helps identify the location of test faults.
> >
> > Cc: Brendan Higgins <brendanhiggins@google.com>
> > Cc: David Gow <davidgow@google.com>
> > Cc: Rae Moar <rmoar@google.com>
> > Cc: Shuah Khan <skhan@linuxfoundation.org>
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > Link: https://lore.kernel.org/r/20240301194037.532117-7-mic@digikod.net
> > ---
> 
> I like the idea of this, but am a little bit worried about how
> confusing it might be, given that the location only updates on those
> particular macros.
> 
> Maybe the answer is to make the __KUNIT_SAVE_LOC() macro, or something
> equivalent, a supported API.
> 
> One possibility would be to have a KUNIT_MARKER() macro. If we really
> wanted to, we could expand it to take a string so we can have a more
> user-friendly KUNIT_MARKER(test, "parsing packet") description of
> where things went wrong. Another could be to extend this to use the
> code tagging framework[1], if that lands.
> 
> That being said, I think this is still an improvement without any of
> those features. I've left a few comments below. Let me know what you
> think.

This patch adds opportunistic markers to test code.  Because an uncaught
fault would be a bug, I think in practice nobody would use
KUNIT_MARKER() explicitly.  I found _KUNIT_SAVE_LOC() to be useful while
writing tests or debugging them.  With this patch, it is still possible
to call KUNIT_SUCCESS() if someone wants to add an explicit mark, and I
think it would make more sense.

> 
> Cheers,
> -- David
> 
> [1]: https://lwn.net/Articles/906660/
> >
> > Changes since v1:
> > * Added Kees's Reviewed-by.
> > ---
> >  include/kunit/test.h  | 24 +++++++++++++++++++++---
> >  lib/kunit/try-catch.c | 10 +++++++---
> >  2 files changed, 28 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index fcb4a4940ace..f3aa66eb0087 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -301,6 +301,8 @@ struct kunit {
> >         struct list_head resources; /* Protected by lock. */
> >
> >         char status_comment[KUNIT_STATUS_COMMENT_SIZE];
> > +       /* Saves the last seen test. Useful to help with faults. */
> > +       struct kunit_loc last_seen;
> >  };
> >
> >  static inline void kunit_set_failure(struct kunit *test)
> > @@ -567,6 +569,15 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt,
> >  #define kunit_err(test, fmt, ...) \
> >         kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
> >
> > +/*
> > + * Must be called at the beginning of each KUNIT_*_ASSERTION().
> > + * Cf. KUNIT_CURRENT_LOC.
> > + */
> > +#define _KUNIT_SAVE_LOC(test) do {                                            \
> > +       WRITE_ONCE(test->last_seen.file, __FILE__);                            \
> > +       WRITE_ONCE(test->last_seen.line, __LINE__);                            \
> > +} while (0)
> 
> Can we get rid of the leading '_', make this public, and document it?
> If we want to rename it to KUNIT_MARKER() or similar, that might work
> better, too.

We can do that but I'm not convinced it would be useful.

> 
> > +
> >  /**
> >   * KUNIT_SUCCEED() - A no-op expectation. Only exists for code clarity.
> >   * @test: The test context object.
> > @@ -575,7 +586,7 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt,
> >   * words, it does nothing and only exists for code clarity. See
> >   * KUNIT_EXPECT_TRUE() for more information.
> >   */
> > -#define KUNIT_SUCCEED(test) do {} while (0)
> > +#define KUNIT_SUCCEED(test) _KUNIT_SAVE_LOC(test)
> >
> >  void __noreturn __kunit_abort(struct kunit *test);
> >
> > @@ -601,14 +612,16 @@ void __kunit_do_failed_assertion(struct kunit *test,
> >  } while (0)
> >
> >
> > -#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...)                     \
> > +#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) do {                \
> > +       _KUNIT_SAVE_LOC(test);                                                 \
> >         _KUNIT_FAILED(test,                                                    \
> >                       assert_type,                                             \
> >                       kunit_fail_assert,                                       \
> >                       kunit_fail_assert_format,                                \
> >                       {},                                                      \
> >                       fmt,                                                     \
> > -                     ##__VA_ARGS__)
> > +                     ##__VA_ARGS__);                                          \
> > +} while (0)
> >
> >  /**
> >   * KUNIT_FAIL() - Always causes a test to fail when evaluated.
> > @@ -637,6 +650,7 @@ void __kunit_do_failed_assertion(struct kunit *test,
> >                               fmt,                                             \
> >                               ...)                                             \
> >  do {                                                                          \
> > +       _KUNIT_SAVE_LOC(test);                                                 \
> >         if (likely(!!(condition_) == !!expected_true_))                        \
> >                 break;                                                         \
> >                                                                                \
> > @@ -698,6 +712,7 @@ do {                                                                               \
> >                 .right_text = #right,                                          \
> >         };                                                                     \
> >                                                                                \
> > +       _KUNIT_SAVE_LOC(test);                                                 \
> >         if (likely(__left op __right))                                         \
> >                 break;                                                         \
> >                                                                                \
> > @@ -758,6 +773,7 @@ do {                                                                               \
> >                 .right_text = #right,                                          \
> >         };                                                                     \
> >                                                                                \
> > +       _KUNIT_SAVE_LOC(test);                                                 \
> >         if (likely((__left) && (__right) && (strcmp(__left, __right) op 0)))   \
> >                 break;                                                         \
> >                                                                                \
> > @@ -791,6 +807,7 @@ do {                                                                               \
> >                 .right_text = #right,                                          \
> >         };                                                                     \
> >                                                                                \
> > +       _KUNIT_SAVE_LOC(test);                                                 \
> >         if (likely(__left && __right))                                         \
> >                 if (likely(memcmp(__left, __right, __size) op 0))              \
> >                         break;                                                 \
> > @@ -815,6 +832,7 @@ do {                                                                               \
> >  do {                                                                          \
> >         const typeof(ptr) __ptr = (ptr);                                       \
> >                                                                                \
> > +       _KUNIT_SAVE_LOC(test);                                                 \
> >         if (!IS_ERR_OR_NULL(__ptr))                                            \
> >                 break;                                                         \
> >                                                                                \
> > diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> > index c6ee4db0b3bd..2ec21c6918f3 100644
> > --- a/lib/kunit/try-catch.c
> > +++ b/lib/kunit/try-catch.c
> > @@ -91,9 +91,13 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context)
> >
> >         if (exit_code == -EFAULT)
> >                 try_catch->try_result = 0;
> > -       else if (exit_code == -EINTR)
> > -               kunit_err(test, "try faulted\n");
> > -       else if (exit_code == -ETIMEDOUT)
> > +       else if (exit_code == -EINTR) {
> > +               if (test->last_seen.file)
> > +                       kunit_err(test, "try faulted after %s:%d\n",
> > +                                 test->last_seen.file, test->last_seen.line);
> 
> It's possibly a bit confusing to just say "after file:line",
> particularly if we then loop or call a function "higher up" in the
> file. Maybe something like "try faulted: last line seen %s:%d" is
> clearer.

OK

> 
> > +               else
> > +                       kunit_err(test, "try faulted before the first test\n");
> 
> I don't like using "test" here, as it introduces ambiguity between
> "kunit tests" and "assertions/expectations" if we call them both
> tests. Maybe just "try faulted" here, or "try faulted (no markers
> seen)" or similar?

I agree that "try faulted" would be enough.  I'm totally OK for you to
update this patch directly. Please let me know if you'd prefer me to
send another version with these fixes though.

Do you plan to merge this patches with the v6.9 merge window?

> 
> 
> > +       } else if (exit_code == -ETIMEDOUT)
> >                 kunit_err(test, "try timed out\n");
> >         else if (exit_code)
> >                 kunit_err(test, "Unknown error: %d\n", exit_code);
> > --
> > 2.44.0
> >
diff mbox series

Patch

diff --git a/include/kunit/test.h b/include/kunit/test.h
index fcb4a4940ace..f3aa66eb0087 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -301,6 +301,8 @@  struct kunit {
 	struct list_head resources; /* Protected by lock. */
 
 	char status_comment[KUNIT_STATUS_COMMENT_SIZE];
+	/* Saves the last seen test. Useful to help with faults. */
+	struct kunit_loc last_seen;
 };
 
 static inline void kunit_set_failure(struct kunit *test)
@@ -567,6 +569,15 @@  void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt,
 #define kunit_err(test, fmt, ...) \
 	kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
 
+/*
+ * Must be called at the beginning of each KUNIT_*_ASSERTION().
+ * Cf. KUNIT_CURRENT_LOC.
+ */
+#define _KUNIT_SAVE_LOC(test) do {					       \
+	WRITE_ONCE(test->last_seen.file, __FILE__);			       \
+	WRITE_ONCE(test->last_seen.line, __LINE__);			       \
+} while (0)
+
 /**
  * KUNIT_SUCCEED() - A no-op expectation. Only exists for code clarity.
  * @test: The test context object.
@@ -575,7 +586,7 @@  void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt,
  * words, it does nothing and only exists for code clarity. See
  * KUNIT_EXPECT_TRUE() for more information.
  */
-#define KUNIT_SUCCEED(test) do {} while (0)
+#define KUNIT_SUCCEED(test) _KUNIT_SAVE_LOC(test)
 
 void __noreturn __kunit_abort(struct kunit *test);
 
@@ -601,14 +612,16 @@  void __kunit_do_failed_assertion(struct kunit *test,
 } while (0)
 
 
-#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...)		       \
+#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) do {		       \
+	_KUNIT_SAVE_LOC(test);						       \
 	_KUNIT_FAILED(test,						       \
 		      assert_type,					       \
 		      kunit_fail_assert,				       \
 		      kunit_fail_assert_format,				       \
 		      {},						       \
 		      fmt,						       \
-		      ##__VA_ARGS__)
+		      ##__VA_ARGS__);					       \
+} while (0)
 
 /**
  * KUNIT_FAIL() - Always causes a test to fail when evaluated.
@@ -637,6 +650,7 @@  void __kunit_do_failed_assertion(struct kunit *test,
 			      fmt,					       \
 			      ...)					       \
 do {									       \
+	_KUNIT_SAVE_LOC(test);						       \
 	if (likely(!!(condition_) == !!expected_true_))			       \
 		break;							       \
 									       \
@@ -698,6 +712,7 @@  do {									       \
 		.right_text = #right,					       \
 	};								       \
 									       \
+	_KUNIT_SAVE_LOC(test);						       \
 	if (likely(__left op __right))					       \
 		break;							       \
 									       \
@@ -758,6 +773,7 @@  do {									       \
 		.right_text = #right,					       \
 	};								       \
 									       \
+	_KUNIT_SAVE_LOC(test);						       \
 	if (likely((__left) && (__right) && (strcmp(__left, __right) op 0)))   \
 		break;							       \
 									       \
@@ -791,6 +807,7 @@  do {									       \
 		.right_text = #right,					       \
 	};								       \
 									       \
+	_KUNIT_SAVE_LOC(test);						       \
 	if (likely(__left && __right))					       \
 		if (likely(memcmp(__left, __right, __size) op 0))	       \
 			break;						       \
@@ -815,6 +832,7 @@  do {									       \
 do {									       \
 	const typeof(ptr) __ptr = (ptr);				       \
 									       \
+	_KUNIT_SAVE_LOC(test);						       \
 	if (!IS_ERR_OR_NULL(__ptr))					       \
 		break;							       \
 									       \
diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
index c6ee4db0b3bd..2ec21c6918f3 100644
--- a/lib/kunit/try-catch.c
+++ b/lib/kunit/try-catch.c
@@ -91,9 +91,13 @@  void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context)
 
 	if (exit_code == -EFAULT)
 		try_catch->try_result = 0;
-	else if (exit_code == -EINTR)
-		kunit_err(test, "try faulted\n");
-	else if (exit_code == -ETIMEDOUT)
+	else if (exit_code == -EINTR) {
+		if (test->last_seen.file)
+			kunit_err(test, "try faulted after %s:%d\n",
+				  test->last_seen.file, test->last_seen.line);
+		else
+			kunit_err(test, "try faulted before the first test\n");
+	} else if (exit_code == -ETIMEDOUT)
 		kunit_err(test, "try timed out\n");
 	else if (exit_code)
 		kunit_err(test, "Unknown error: %d\n", exit_code);