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

[sashiko-bot@kernel.org]

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()?

> +	}
>  
>  	/* Then installed path. */
>  	snprintf(path_dir, PATH_MAX, "%s/tests", get_argv_exec_path());

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012053013.14378-1-namhyung@kernel.org?part=4