Re: [PATCH bpf-next v2 7/7] selftests/bpf: Skip compare_text subtest when stdout is not saved

[bot+bpf-ci]

> selftests/bpf: Skip compare_text subtest when stdout is not saved

TL;DR: Nit only: the new test__skip() in the compare_text_mismatch subtest
gives no reason for skipping; a short comment or stderr message might help.
The skip logic itself looks correct.

> diff --git a/tools/testing/selftests/bpf/prog_tests/prog_tests_framework.c b/tools/testing/selftests/bpf/prog_tests/prog_tests_framework.c
> index b4cd7c7ff439..ddb6cdc1579f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/prog_tests_framework.c
> +++ b/tools/testing/selftests/bpf/prog_tests/prog_tests_framework.c

[ ... ]

> @@ -203,14 +203,18 @@ void test_prog_tests_framework_compare_text(void)
>  	}
>
>  	if (test__start_subtest("compare_text_mismatch")) {
> -		err = compare_text_to_expected("line two\n", "line one\n");
> -		fflush(stdout);
> +		if (!env.subtest_state->stdout_saved) {
> +			test__skip();
> +		} else {
> +			err = compare_text_to_expected("line two\n", "line one\n");
> +			fflush(stdout);
>
> -		ASSERT_EQ(err, -1, "mismatch_rc");
> -		ASSERT_HAS_SUBSTR(env.subtest_state->log_buf, "-line one",
> -				  "diff_has_expected");
> -		ASSERT_HAS_SUBSTR(env.subtest_state->log_buf, "+line two",
> -				  "diff_has_actual");
> +			ASSERT_EQ(err, -1, "mismatch_rc");
> +			ASSERT_HAS_SUBSTR(env.subtest_state->log_buf, "-line one",
> +					  "diff_has_expected");
> +			ASSERT_HAS_SUBSTR(env.subtest_state->log_buf, "+line two",
> +					  "diff_has_actual");
> +		}
>  		test__end_subtest();
>  	}
>  }

This isn't a bug, but could the skip in the compare_text_mismatch subtest
carry an explanation?

	if (!env.subtest_state->stdout_saved) {
		test__skip();
	}

A subsystem guideline flags selftests that skip silently with no message,
and test__skip() here prints nothing about why the subtest was skipped.
The same condition covers -v in non-parallel mode, a libc without
open_memstream() support, and an open_memstream() failure, so the reason is
not obvious from the test output alone.

For context, the skip itself looks correct: env.subtest_state is non-NULL
inside the subtest, test__skip() sets subtest_state->skipped, and
test__end_subtest() handles a NULL stdout_saved.

The other skips in this file are also bare test__skip() calls, and
test__skip() takes no reason argument, so the guideline may not map cleanly
onto test_progs.  If a reason is wanted, would a short comment above the
test__skip() call, or a separate message printed to env.stderr_saved, be
appropriate?


---
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/38559567568