diff mbox series

[v2,2/4] selftests/mm: Add helper for logging test start and results

Message ID 20250527-selftests-mm-cow-dedupe-v2-2-ff198df8e38e@kernel.org
State New
Headers show
Series selftests/mm: cow and gup_longterm cleanups | expand

Commit Message

Mark Brown May 27, 2025, 4:04 p.m. UTC
Several of the MM tests have a pattern of printing a description of the
test to be run then reporting the actual TAP result using a generic string
not connected to the specific test, often in a shared function used by many
tests. The name reported typically varies depending on the specific result
rather than the test too. This causes problems for tooling that works with
test results, the names reported with the results are used to deduplicate
tests and track them between runs so both duplicated names and changing
names cause trouble for things like UIs and automated bisection.

As a first step towards matching these tests better with the expectations
of kselftest provide helpers which record the test name as part of the
initial print and then use that as part of reporting a result.

This is not added as a generic kselftest helper partly because the use of
a variable to store the test name doesn't fit well with the header only
implementation of kselftest.h and partly because it's not really an
intended pattern. Ideally at some point the mm tests that use it will be
updated to not need it.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 tools/testing/selftests/mm/vm_util.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

David Hildenbrand June 3, 2025, 12:37 p.m. UTC | #1
On 27.05.25 18:04, Mark Brown wrote:
> Several of the MM tests have a pattern of printing a description of the
> test to be run then reporting the actual TAP result using a generic string
> not connected to the specific test, often in a shared function used by many
> tests. The name reported typically varies depending on the specific result
> rather than the test too. This causes problems for tooling that works with
> test results, the names reported with the results are used to deduplicate
> tests and track them between runs so both duplicated names and changing
> names cause trouble for things like UIs and automated bisection.
> 
> As a first step towards matching these tests better with the expectations
> of kselftest provide helpers which record the test name as part of the
> initial print and then use that as part of reporting a result.
> 
> This is not added as a generic kselftest helper partly because the use of
> a variable to store the test name doesn't fit well with the header only
> implementation of kselftest.h and partly because it's not really an
> intended pattern. Ideally at some point the mm tests that use it will be
> updated to not need it.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>   tools/testing/selftests/mm/vm_util.h | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h
> index 6effafdc4d8a..4944e4c79051 100644
> --- a/tools/testing/selftests/mm/vm_util.h
> +++ b/tools/testing/selftests/mm/vm_util.h
> @@ -3,6 +3,7 @@
>   #include <stdbool.h>
>   #include <sys/mman.h>
>   #include <err.h>
> +#include <stdarg.h>
>   #include <strings.h> /* ffsl() */
>   #include <unistd.h> /* _SC_PAGESIZE */
>   #include "../kselftest.h"
> @@ -74,6 +75,25 @@ int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len,
>   unsigned long get_free_hugepages(void);
>   bool check_vmflag_io(void *addr);
>   
> +/* These helpers need to be inline to match the kselftest.h idiom. */
> +static char test_name[1024];
> +
> +static inline void log_test_start(const char *name, ...)
> +{
> +	va_list args;
> +	va_start(args, name);
> +
> +	vsnprintf(test_name, sizeof(test_name), name, args);
> +	ksft_print_msg("[RUN] %s\n", test_name);
> +
> +	va_end(args);
> +}
> +
> +static inline void log_test_result(int result)
> +{
> +	ksft_test_result_report(result, "%s\n", test_name);
> +}

Won't win a beauty contest, but should get the job done.

We could allocate the array in log_test_start() and free it in 
log_test_result(). Then, we could assert more easily that we always have 
a log_test_result() follow exactly one log_test_start() etc.
Mark Brown June 3, 2025, 6:27 p.m. UTC | #2
On Tue, Jun 03, 2025 at 02:37:41PM +0200, David Hildenbrand wrote:
> On 27.05.25 18:04, Mark Brown wrote:

> > +static char test_name[1024];
> > +
> > +static inline void log_test_start(const char *name, ...)
> > +{
> > +	va_list args;
> > +	va_start(args, name);
> > +
> > +	vsnprintf(test_name, sizeof(test_name), name, args);
> > +	ksft_print_msg("[RUN] %s\n", test_name);

> We could allocate the array in log_test_start() and free it in
> log_test_result(). Then, we could assert more easily that we always have a
> log_test_result() follow exactly one log_test_start() etc.

We could, however we don't have vasprintf() in nolibc and people have
been doing work towards making nolibc more generally useful as a libc
for the selftests (and/or the selftest interfaces more friendly to
nolibc).  I don't really know what the end goal with that is but given
the fairly small gain and the hope that this won't be a long term
framework for anything I'd rather not add something that gets in the way
of whatever's going on there.

Ideally the test programs would be refactored and these helpers deleted,
but as we said previously that's a bigger job that neither of us is
likely to get to in the short term :(
diff mbox series

Patch

diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h
index 6effafdc4d8a..4944e4c79051 100644
--- a/tools/testing/selftests/mm/vm_util.h
+++ b/tools/testing/selftests/mm/vm_util.h
@@ -3,6 +3,7 @@ 
 #include <stdbool.h>
 #include <sys/mman.h>
 #include <err.h>
+#include <stdarg.h>
 #include <strings.h> /* ffsl() */
 #include <unistd.h> /* _SC_PAGESIZE */
 #include "../kselftest.h"
@@ -74,6 +75,25 @@  int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len,
 unsigned long get_free_hugepages(void);
 bool check_vmflag_io(void *addr);
 
+/* These helpers need to be inline to match the kselftest.h idiom. */
+static char test_name[1024];
+
+static inline void log_test_start(const char *name, ...)
+{
+	va_list args;
+	va_start(args, name);
+
+	vsnprintf(test_name, sizeof(test_name), name, args);
+	ksft_print_msg("[RUN] %s\n", test_name);
+
+	va_end(args);
+}
+
+static inline void log_test_result(int result)
+{
+	ksft_test_result_report(result, "%s\n", test_name);
+}
+
 /*
  * On ppc64 this will only work with radix 2M hugepage size
  */