Re: [PATCH v2 12/14] perf test: Add -j/--junit option for JUnit XML test reports

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Tests failing with arbitrary error codes or signals are logged as PASSED in the generated JUnit XML report.
- [Medium] `xml_escape` silently drops UTF-8 and extended ASCII characters on architectures where `char` is signed.
--

commit 85c89b056c0810130f9d5935a708601da2cc88ff
Author: Ian Rogers <irogers@google.com>

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

Add a -j/--junit command line option to generate standard JUnit XML format
test reports. The XML report captures individual test suite and subtest
execution latency alongside XML-escaped failure logs and skip reasons.

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index fb4dffacf4068..38697243f33ad 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c

[ ... ]

> @@ -367,8 +373,34 @@ static unsigned int summary_tests_skipped;
>  static unsigned int summary_tests_failed;
>  static struct strbuf summary_failed_tests_buf = STRBUF_INIT;
>  
> +static char *xml_escape(const char *str)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	const char *p;
> +	char *res;
> +
> +	if (!str)
> +		return strdup("");
> +
> +	for (p = str; *p; p++) {
> +		if (*p == '&')
> +			strbuf_addstr(&buf, "&amp;");
> +		else if (*p == '<')
> +			strbuf_addstr(&buf, "&lt;");
> +		else if (*p == '>')
> +			strbuf_addstr(&buf, "&gt;");
> +		else if (*p == '"')
> +			strbuf_addstr(&buf, "&quot;");
> +		else if (*p >= 32 || *p == '\n' || *p == '\t')

[Severity: Medium]
Since char is signed on architectures like x86, will characters with the
high bit set (such as UTF-8 sequences) evaluate as negative values? 

It looks like they would fail the *p >= 32 check and be silently dropped from
the XML output. Could *p be cast to an unsigned char for this comparison?

> +			strbuf_addch(&buf, *p);
> +	}
> +	res = strbuf_detach(&buf, NULL);
> +	return res ? res : strdup("");
> +}

[ ... ]

> @@ -416,6 +448,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: Medium]
Will this strict check for TEST_FAIL miss tests that fail with other negative
POSIX error codes like -ENOMEM? 

The switch statement above this code falls through to default: for any
unrecognized error code, treating them as failures for the terminal output.
However, if a test returns a different error code, the <failure> element
would be omitted here, potentially causing CI systems and JUnit parsers to
report a broken 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);

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