Re: [PATCH v2 4/4] perf test: Update PE file test for other locations

[Namhyung Kim <namhyung@kernel.org>]

On Mon, Oct 12, 2026 at 05:38:20AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] perf/tests: spurious test failure due to brittle directory checking
> --
> 
> commit 073d3f0f55cefe06ae3b0caf163e2a6505e656b3
> Author: Namhyung Kim <namhyung@kernel.org>
> 
> perf test: Update PE file test for other locations
> 
> This updates test__pe_file_parsing() to search multiple directories to
> find the tests directory, similar to shell_tests__dir_fd(), and adds
> debug messages when skipped.
> 
> > diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
> > index 3979f38bf127c..78dae7e43603b 100644
> > --- a/tools/perf/tests/pe-file-parsing.c
> > +++ b/tools/perf/tests/pe-file-parsing.c
> > @@ -74,10 +74,17 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
> >  {
> >  	struct stat st;
> >  	char path_dir[PATH_MAX];
> > +	static const char * const tests_dirs[] = {
> > +		"./tools/perf/tests",
> > +		"./tests",
> > +		"./source/tests"
> > +	};
> >  
> >  	/* First try development tree tests. */
> > -	if (!lstat("./tests", &st))
> > -		return run_dir("./tests");
> > +	for (size_t i = 0; i < ARRAY_SIZE(tests_dirs); ++i) {
> > +		if (!lstat(tests_dirs[i], &st))
> > +			return run_dir(tests_dirs[i]);
> 
> [Severity: Medium]
> Will this cause a spurious test failure if a matching directory exists but
> doesn't contain pe-file.exe?
> 
> The new loop eagerly calls run_dir() upon finding a matching directory
> name instead of verifying the actual test file exists.
> 
> If pe-file.exe is missing from the matched directory, run_dir() calls
> filename__read_build_id() which returns an error. This causes
> TEST_ASSERT_VAL to immediately return TEST_FAIL, breaking the test suite
> instead of falling back to the installed path.
> 
> The commit message notes this logic tries other paths like
> shell_tests__dir_fd(), but shell_tests__dir_fd() merely returns a directory
> descriptor and gracefully continues with zero shell tests if the directory
> is empty. By contrast, run_dir() explicitly asserts file validity and
> aborts the suite with a hard TEST_FAIL.
> 
> Should this verify pe-file.exe actually exists before committing to a
> directory and calling run_dir()?

It's expected to point to the tests directory in the source tree, so it
would contain the file.  But in a rare case where it runs on a directory
which happens to have a "tests", the test will fail on reading build-id.

I think it's a minor pre-existing issue and maybe we can just ignore it.

Thanks,
Namhyung