kunit: test: Move fault tests behind KUNIT_FAULT_TEST Kconfig option

[PATCH] kunit: test: Move fault tests behind KUNIT_FAULT_TEST Kconfig option

Posted by David Gow 1 week, 3 days ago

The NULL dereference tests in kunit_fault deliberately trigger a kernel
BUG(), and therefore print the associated stack trace, even when the
test passes. This is both annoying (as it bloats the test output), and
can confuse some test harnesses, which assume any BUG() is a failure.

Allow these tests to be specifically disabled (without disabling all
of KUnit's other tests), by placing them behind the
CONFIG_KUNIT_FAULT_TEST Kconfig option. This is enabled by default, but
can be set to 'n' to disable the test. An empty 'kunit_fault' suite is
left behind, which will automatically be marked 'skipped'.

As the fault tests already were disabled under UML (as they weren't
compatible with its fault handling), we can simply adapt those
conditions, and add a dependency on !UML for our new option.

Suggested-by: Guenter Roeck <linux@roeck-us.net>
Link: https://lore.kernel.org/all/928249cc-e027-4f7f-b43f-502f99a1ea63@roeck-us.net/
Fixes: 82b0beff3497 ("kunit: Add tests for fault")
Signed-off-by: David Gow <davidgow@google.com>
---
 lib/kunit/Kconfig      | 11 +++++++++++
 lib/kunit/kunit-test.c |  8 ++++----
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
index 68a6daec0aef..34d7242d526d 100644
--- a/lib/kunit/Kconfig
+++ b/lib/kunit/Kconfig
@@ -24,6 +24,17 @@ config KUNIT_DEBUGFS
 	  test suite, which allow users to see results of the last test suite
 	  run that occurred.
 
+config KUNIT_FAULT_TEST
+	bool "Enable KUnit tests which print BUG stacktraces"
+	depends on KUNIT_TEST
+	depends on !UML
+	default y
+	help
+	  Enables fault handling tests for the KUnit framework. These tests may
+	  trigger a kernel BUG(), and the associated stack trace, even when they
+	  pass. If this conflicts with your test infrastrcture (or is confusing
+	  or annoying), they can be disabled by setting this to N.
+
 config KUNIT_TEST
 	tristate "KUnit test for KUnit" if !KUNIT_ALL_TESTS
 	default KUNIT_ALL_TESTS
diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
index 0fdca5fffaec..e3412e0ca399 100644
--- a/lib/kunit/kunit-test.c
+++ b/lib/kunit/kunit-test.c
@@ -109,7 +109,7 @@ static struct kunit_suite kunit_try_catch_test_suite = {
 	.test_cases = kunit_try_catch_test_cases,
 };
 
-#ifndef CONFIG_UML
+#if IS_ENABLED(CONFIG_KUNIT_FAULT_TEST)
 
 static void kunit_test_null_dereference(void *data)
 {
@@ -136,12 +136,12 @@ static void kunit_test_fault_null_dereference(struct kunit *test)
 	KUNIT_EXPECT_TRUE(test, ctx->function_called);
 }
 
-#endif /* !CONFIG_UML */
+#endif /* CONFIG_KUNIT_FAULT_TEST */
 
 static struct kunit_case kunit_fault_test_cases[] = {
-#ifndef CONFIG_UML
+#if IS_ENABLED(CONFIG_KUNIT_FAULT_TEST)
 	KUNIT_CASE(kunit_test_fault_null_dereference),
-#endif /* !CONFIG_UML */
+#endif /* CONFIG_KUNIT_FAULT_TEST */
 	{}
 };
 
-- 
2.44.0.769.g3c40516874-goog

Re: [PATCH] kunit: test: Move fault tests behind KUNIT_FAULT_TEST Kconfig option

Posted by Rae Moar 1 week, 3 days ago

On Tue, Apr 23, 2024 at 5:08 AM David Gow <davidgow@google.com> wrote:
>
> The NULL dereference tests in kunit_fault deliberately trigger a kernel
> BUG(), and therefore print the associated stack trace, even when the
> test passes. This is both annoying (as it bloats the test output), and
> can confuse some test harnesses, which assume any BUG() is a failure.
>
> Allow these tests to be specifically disabled (without disabling all
> of KUnit's other tests), by placing them behind the
> CONFIG_KUNIT_FAULT_TEST Kconfig option. This is enabled by default, but
> can be set to 'n' to disable the test. An empty 'kunit_fault' suite is
> left behind, which will automatically be marked 'skipped'.
>
> As the fault tests already were disabled under UML (as they weren't
> compatible with its fault handling), we can simply adapt those
> conditions, and add a dependency on !UML for our new option.
>
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Link: https://lore.kernel.org/all/928249cc-e027-4f7f-b43f-502f99a1ea63@roeck-us.net/
> Fixes: 82b0beff3497 ("kunit: Add tests for fault")
> Signed-off-by: David Gow <davidgow@google.com>

Hello!

This looks good to me. Very useful!

Reviewed-by: Rae Moar <rmoar@google.com>

Thanks!
-Rae

> ---
>  lib/kunit/Kconfig      | 11 +++++++++++
>  lib/kunit/kunit-test.c |  8 ++++----
>  2 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
> index 68a6daec0aef..34d7242d526d 100644
> --- a/lib/kunit/Kconfig
> +++ b/lib/kunit/Kconfig
> @@ -24,6 +24,17 @@ config KUNIT_DEBUGFS
>           test suite, which allow users to see results of the last test suite
>           run that occurred.
>
> +config KUNIT_FAULT_TEST
> +       bool "Enable KUnit tests which print BUG stacktraces"
> +       depends on KUNIT_TEST
> +       depends on !UML
> +       default y
> +       help
> +         Enables fault handling tests for the KUnit framework. These tests may
> +         trigger a kernel BUG(), and the associated stack trace, even when they
> +         pass. If this conflicts with your test infrastrcture (or is confusing
> +         or annoying), they can be disabled by setting this to N.
> +
>  config KUNIT_TEST
>         tristate "KUnit test for KUnit" if !KUNIT_ALL_TESTS
>         default KUNIT_ALL_TESTS
> diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
> index 0fdca5fffaec..e3412e0ca399 100644
> --- a/lib/kunit/kunit-test.c
> +++ b/lib/kunit/kunit-test.c
> @@ -109,7 +109,7 @@ static struct kunit_suite kunit_try_catch_test_suite = {
>         .test_cases = kunit_try_catch_test_cases,
>  };
>
> -#ifndef CONFIG_UML
> +#if IS_ENABLED(CONFIG_KUNIT_FAULT_TEST)
>
>  static void kunit_test_null_dereference(void *data)
>  {
> @@ -136,12 +136,12 @@ static void kunit_test_fault_null_dereference(struct kunit *test)
>         KUNIT_EXPECT_TRUE(test, ctx->function_called);
>  }
>
> -#endif /* !CONFIG_UML */
> +#endif /* CONFIG_KUNIT_FAULT_TEST */
>
>  static struct kunit_case kunit_fault_test_cases[] = {
> -#ifndef CONFIG_UML
> +#if IS_ENABLED(CONFIG_KUNIT_FAULT_TEST)
>         KUNIT_CASE(kunit_test_fault_null_dereference),
> -#endif /* !CONFIG_UML */
> +#endif /* CONFIG_KUNIT_FAULT_TEST */
>         {}
>  };
>
> --
> 2.44.0.769.g3c40516874-goog
>

Re: [PATCH] kunit: test: Move fault tests behind KUNIT_FAULT_TEST Kconfig option

Posted by Mickaël Salaün 1 week, 3 days ago

On Tue, Apr 23, 2024 at 05:08:06PM +0800, David Gow wrote:
> The NULL dereference tests in kunit_fault deliberately trigger a kernel
> BUG(), and therefore print the associated stack trace, even when the
> test passes. This is both annoying (as it bloats the test output), and
> can confuse some test harnesses, which assume any BUG() is a failure.
> 
> Allow these tests to be specifically disabled (without disabling all
> of KUnit's other tests), by placing them behind the
> CONFIG_KUNIT_FAULT_TEST Kconfig option. This is enabled by default, but
> can be set to 'n' to disable the test. An empty 'kunit_fault' suite is
> left behind, which will automatically be marked 'skipped'.
> 
> As the fault tests already were disabled under UML (as they weren't
> compatible with its fault handling), we can simply adapt those
> conditions, and add a dependency on !UML for our new option.
> 
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Link: https://lore.kernel.org/all/928249cc-e027-4f7f-b43f-502f99a1ea63@roeck-us.net/
> Fixes: 82b0beff3497 ("kunit: Add tests for fault")
> Signed-off-by: David Gow <davidgow@google.com>

Good idea!

Reviewed-by: Mickaël Salaün <mic@digikod.net>

> ---
>  lib/kunit/Kconfig      | 11 +++++++++++
>  lib/kunit/kunit-test.c |  8 ++++----
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
> index 68a6daec0aef..34d7242d526d 100644
> --- a/lib/kunit/Kconfig
> +++ b/lib/kunit/Kconfig
> @@ -24,6 +24,17 @@ config KUNIT_DEBUGFS
>  	  test suite, which allow users to see results of the last test suite
>  	  run that occurred.
>  
> +config KUNIT_FAULT_TEST
> +	bool "Enable KUnit tests which print BUG stacktraces"
> +	depends on KUNIT_TEST
> +	depends on !UML
> +	default y
> +	help
> +	  Enables fault handling tests for the KUnit framework. These tests may
> +	  trigger a kernel BUG(), and the associated stack trace, even when they
> +	  pass. If this conflicts with your test infrastrcture (or is confusing
> +	  or annoying), they can be disabled by setting this to N.
> +
>  config KUNIT_TEST
>  	tristate "KUnit test for KUnit" if !KUNIT_ALL_TESTS
>  	default KUNIT_ALL_TESTS
> diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
> index 0fdca5fffaec..e3412e0ca399 100644
> --- a/lib/kunit/kunit-test.c
> +++ b/lib/kunit/kunit-test.c
> @@ -109,7 +109,7 @@ static struct kunit_suite kunit_try_catch_test_suite = {
>  	.test_cases = kunit_try_catch_test_cases,
>  };
>  
> -#ifndef CONFIG_UML
> +#if IS_ENABLED(CONFIG_KUNIT_FAULT_TEST)
>  
>  static void kunit_test_null_dereference(void *data)
>  {
> @@ -136,12 +136,12 @@ static void kunit_test_fault_null_dereference(struct kunit *test)
>  	KUNIT_EXPECT_TRUE(test, ctx->function_called);
>  }
>  
> -#endif /* !CONFIG_UML */
> +#endif /* CONFIG_KUNIT_FAULT_TEST */
>  
>  static struct kunit_case kunit_fault_test_cases[] = {
> -#ifndef CONFIG_UML
> +#if IS_ENABLED(CONFIG_KUNIT_FAULT_TEST)
>  	KUNIT_CASE(kunit_test_fault_null_dereference),
> -#endif /* !CONFIG_UML */
> +#endif /* CONFIG_KUNIT_FAULT_TEST */
>  	{}
>  };
>  
> -- 
> 2.44.0.769.g3c40516874-goog
>

Re: [PATCH] kunit: test: Move fault tests behind KUNIT_FAULT_TEST Kconfig option

Posted by Guenter Roeck 1 week, 3 days ago

On Tue, Apr 23, 2024 at 05:08:06PM +0800, David Gow wrote:
> The NULL dereference tests in kunit_fault deliberately trigger a kernel
> BUG(), and therefore print the associated stack trace, even when the
> test passes. This is both annoying (as it bloats the test output), and
> can confuse some test harnesses, which assume any BUG() is a failure.
> 
> Allow these tests to be specifically disabled (without disabling all
> of KUnit's other tests), by placing them behind the
> CONFIG_KUNIT_FAULT_TEST Kconfig option. This is enabled by default, but
> can be set to 'n' to disable the test. An empty 'kunit_fault' suite is
> left behind, which will automatically be marked 'skipped'.
> 
> As the fault tests already were disabled under UML (as they weren't
> compatible with its fault handling), we can simply adapt those
> conditions, and add a dependency on !UML for our new option.
> 
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Link: https://lore.kernel.org/all/928249cc-e027-4f7f-b43f-502f99a1ea63@roeck-us.net/
> Fixes: 82b0beff3497 ("kunit: Add tests for fault")
> Signed-off-by: David Gow <davidgow@google.com>

Reviewed-and-tested-by: Guenter Roeck <linux@roeck-us.net>

Thanks!
Guenter