Re: [PATCH bpf-next v2 2/3] selftests/bpf: report placeholder tests as SKIP, not OK

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2766 bytes --]

> diff --git a/tools/testing/selftests/bpf/progs/verifier_ldsx.c b/tools/testing/selftests/bpf/progs/verifier_ldsx.c
> index 41340877dc9d..e58ea69e3854 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_ldsx.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_ldsx.c
> @@ -436,6 +436,7 @@ void kfunc_root(void)
>
>  SEC("socket")
>  __description("cpuv4 is not supported by compiler or jit, use a dummy test")
> +__skip("cpuv4 is not supported by compiler or jit")
>  __success
>  int dummy_test(void)
>  {

This isn't a bug, but since the SKIP line is already labelled with the
__description text, would it read better for __skip(reason) to supply a
distinct message rather than repeating the same wording? Looking at all
15 converted placeholders (arena_kfunc.c, stack_arg_fail.c,
stack_arg_precision.c, verifier_aggregate_ret.c, verifier_bswap.c,
verifier_gotol.c, verifier_ldsx.c, verifier_load_acquire.c,
verifier_movsx.c, verifier_percpu_addr.c, verifier_private_stack.c,
verifier_sdiv.c, verifier_stack_arg.c, verifier_stack_arg_order.c,
verifier_store_release.c), each has __skip() repeating the adjacent
__description() nearly verbatim, differing only by a trailing clause like
", use a dummy test" or ", dummy test". This means each file now stores
the reason twice and the two strings can drift apart independently.

> diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
> index 07807757b518..794a7dfb0579 100644
> --- a/tools/testing/selftests/bpf/test_loader.c
> +++ b/tools/testing/selftests/bpf/test_loader.c
> @@ -1327,6 +1330,12 @@ void run_subtest(struct test_loader *tester,
>  	if (!test__start_subtest_with_desc(subspec->name, subspec->description))
>  		return;
>
> +	if (spec->skip_reason) {
> +		printf("%s:SKIP: %s\n", __func__, spec->skip_reason);
> +		test__skip();
> +		return;
> +	}
> +
>  	if ((get_current_arch() & spec->arch_mask) == 0) {
>  		test__skip();
>  		return;

This isn't a bug, but is the extra reason line reachable in a normal
(non -v) run? Since test__start_subtest_with_desc() has already called
stdio_hijack_init(), this printf() goes into subtest_state->log_buf.
Looking at dump_test_log() in test_progs.c, the log_buf is only emitted
when verbose() || force_log || subtest_failed is true, which would be
false for a skipped subtest in a default run. Meanwhile the SKIP verdict
itself is printed by print_subtest_name() using the __description text,
so in the default output only the subtest name appears next to :SKIP.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32778075708