Re: [PATCH 13/15] perf test: Add -j/--junit option for JUnit XML test reports

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] JUnit XML incorrectly reports failed tests as passed if they return negative error codes other than TEST_FAIL (-1) or crash.
--

commit 0cfa01c83fa8f407962850a84df883fa8c3c4967
Author: Ian Rogers <irogers@google.com>

perf test: Add -j/--junit option for JUnit XML test reports

This commit adds a -j/--junit command-line option to generate standard
JUnit XML format test reports. The report captures test execution latency,
XML-escaped failure logs, and skip reasons while preserving the concurrency
of parallel test execution.

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c

[ ... ]

> @@ -421,6 +453,34 @@ static int print_test_result(struct test_suite *t, int curr_suite, int curr_test
>  		break;
>  	}
>  
> +	if (junit_filename && result != TEST_RUNNING) {
> +		const char *classname = t->desc;
> +		const char *testname = test_description(t, curr_test_case);
> +		char *escaped_err = xml_escape(err_output);
> +		char *escaped_class = xml_escape(classname);
> +		char *escaped_test = xml_escape(testname);
> +
> +		strbuf_addf(&junit_xml_buf,
> +			    "    <testcase classname=\"%s\" name=\"%s\" time=\"%.2f\">\n",
> +			    escaped_class, escaped_test, elapsed);
> +		if (result == TEST_FAIL) {

[Severity: High]
Does this incorrectly report tests as passed if they return negative error
codes other than TEST_FAIL (-1) or crash?

In print_test_result(), any result that is not TEST_RUNNING, TEST_OK, or
TEST_SKIP correctly falls through to the default case in the earlier switch
statement and increments summary_tests_failed.

However, this XML generation block explicitly checks if result == TEST_FAIL.
If a test crashes (returning a signal status) or returns another negative
error code like -ENOMEM, will the <testcase> element be closed without a
<failure> tag, causing CI systems to silently treat the failed test as
passed?

> +			strbuf_addf(&junit_xml_buf,
> +				    "      <failure message=\"FAILED\">\n%s\n      </failure>\n",
> +				    escaped_err);
> +		} else if (result == TEST_SKIP) {
> +			const char *reason = skip_reason(t, curr_test_case);
> +			char *escaped_reason = xml_escape(reason ? reason : "Skip");
> +
> +			strbuf_addf(&junit_xml_buf, "      <skipped message=\"%s\"/>\n",
> +				    escaped_reason);
> +			free(escaped_reason);
> +		}
> +		strbuf_addstr(&junit_xml_buf, "    </testcase>\n");
> +		free(escaped_err);
> +		free(escaped_class);
> +		free(escaped_test);
> +	}
> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601061401.1541457-1-irogers@google.com?part=13