Received-SPF: pass (google.com: domain of linux-kernel+bounces-155648-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 23 Apr 2024 19:07:51 +0200
From: =?utf-8?Q?Micka=C3=ABl_Sala=C3=BCn?= <mic@digikod.net>
To: David Gow <davidgow@google.com>
Cc: Rae Moar <rmoar@google.com>, Shuah Khan <skhan@linuxfoundation.org>, 
	Guenter Roeck <linux@roeck-us.net>, kunit-dev@googlegroups.com, linux-kselftest@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] kunit: test: Move fault tests behind KUNIT_FAULT_TEST
 Kconfig option
Message-ID: <20240423.shuwee5Baesh@digikod.net>
References: <20240423090808.242389-1-davidgow@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240423090808.242389-1-davidgow@google.com>

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
>