On Wed, 2026-10-14 at 12:08 +0000, [email protected] wrote: > > selftests/bpf: Skip expected_msgs subtests when stdout is not saved > > TL;DR: Nit only: the new test__skip() in the expected_msgs selftest is silent, > so -v output doesn't say why subtests were skipped; the review asks whether to > print a short reason. 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 53347f379924..b4cd7c7ff439 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/prog_tests_framework.c > > +++ b/tools/testing/selftests/bpf/prog_tests/prog_tests_framework.c > > [ ... ] > > > @@ -169,6 +169,12 @@ void test_prog_tests_framework_expected_msgs(void) > > > > for (i = 0; i < ARRAY_SIZE(cases); i++) { > > if (test__start_subtest(cases[i].name)) { > > + if (!env.subtest_state->stdout_saved) { > > + test__skip(); > > + test__end_subtest(); > > + continue; > > + } > > + > > This isn't a bug, but a subsystem guideline flags a test__skip() that is > issued silently, with no message. > > test__skip() here is called without any explanation, so a -v run only shows > the generic "#N/M <subtest>:SKIP" line from test__end_subtest(). Someone > reading that output has to look at the code or the commit message to learn > the subtests were skipped because stdout is not captured in non-parallel > verbose mode. > > The skip itself looks correct, since stdio_hijack_init() returns early when > verbose() && env.worker_id == -1 and leaves subtest_state->stdout_saved and > log_buf unset. test__skip() takes no reason argument, and many other > test__skip() callers in prog_tests are also bare. > > Would it be worth printing a short reason before test__skip(), for example: > > fprintf(stderr, "SKIP: stdout is not saved in non-parallel verbose > mode\n");
I don't think we need this. for two reasons: a) -v is mainly used for debugging, showing details; this is a test for the framework, not the real bpf test, all tests should pass, no need to debug; b) this is a loop. if we keep adding this message, it could repeat for 9 lines. > > > error_cnt = env.subtest_state->error_cnt; > > msgs.patterns = cases[i].pats; > > msgs.cnt = 0; > > > --- > 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
