On Wed, 2026-10-14 at 12:08 +0000, [email protected] wrote: > > 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? > just one line for this, we can print why the test skipped. and short comment for patch#6 > > --- > 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
-- Thanks, KaFai
