Re: [PATCH bpf-next v2 6/7] selftests/bpf: Skip expected_msgs subtests when stdout is not saved

[KaFai Wan]

On Wed, 2026-10-14 at 12:08 +0000, bot+bpf-ci@xxxxxxxxxx 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