Fail loudly when junit.xml is missing, empty, or unusable - #339
Conversation
process_junit_xml() had several weak failure paths: empty input returned an empty string, which the reporting API only rejected much later with a confusing JSON validation error and a wasted upload; valid but unusable XML, such as an empty testsuites element or a root level testsuite, produced a payload with empty counts that uploaded cleanly and rendered with no diagnostics; a payload carrying a test count without failure and error counts casts to zero downstream and displays as Passed; and invalid XML was not checked before later property and xpath access, producing a fatal error with no explanation of the cause. That last case is how reporting broke for every host in April 2026 (WordPress#310), when email test data containing raw invalid UTF-8 bytes made junit.xml unparseable. In every case the host saw either a clean exit or an unexplained crash, so nothing pointed at the real problem. This is issue WordPress#311. report.php now stops with a clear error when junit.xml is missing or unreadable, and process_junit_xml() stops when the XML is empty, cannot be parsed, or parses without the tests, failures, and errors counts together. The unparseable case includes the first libxml parser error and line number, since that is the hardest one to diagnose from a host. The libxml error handler state is restored either way. A root level testsuite element, a legitimate JUnit shape from producers other than PHPUnit, now parses to correct counts and failure details where it previously produced empty strings. A junit file that genuinely reports zero tests still passes through unchanged, and the reporter displays that as Errored rather than Passed. Also removes an unreachable duplicated return statement. Fixes WordPress#311.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
@ekamran Did you use AI to assist this PR? If so, our new policy is that anything that has had AI assistance be disclosed. On the CI point -- I removed |
|
Yes, I used AI assistance and I have added the disclosure to the PR description. On |
2 participants
Fixes #311.
process_junit_xml() had several weak failure paths: empty input returned an empty string, which the reporting API only rejected much later with a confusing JSON validation error and a wasted upload; valid but unusable XML, such as an empty testsuites element or a root level testsuite, produced a payload with empty counts that uploaded cleanly and rendered with no diagnostics; a payload carrying a test count without failure and error counts casts to zero downstream and displays as Passed; and invalid XML was not checked before later property and xpath access, producing a fatal error with no explanation of the cause. That last case is how reporting broke for every host in April 2026 (#310), when email test data containing raw invalid UTF-8 bytes made junit.xml unparseable. In every case the host saw either a clean exit or an unexplained crash, so nothing pointed at the real problem.
With this change, report.php stops with a clear error when junit.xml does not exist or is not readable, and process_junit_xml() stops when the XML is empty, cannot be parsed, or parses without usable result counts. The unparseable case includes the first libxml parser error and line number, since that is the hardest one to diagnose from a host. The libxml error handler state is restored either way.
The parsed counts guard requires the tests, failures, and errors attributes together, because a payload with a test count but missing failure and error counts casts to zero downstream and would display as Passed. A root level testsuite element, a legitimate JUnit shape from producers other than PHPUnit, now parses to correct counts and failure details where it previously produced empty strings. A junit file that genuinely reports zero tests still passes through unchanged, and the reporter already displays that as Errored rather than Passed.
Verified with a local battery rather than CI, because this repository's workflow runs report.php with continue-on-error, so CI only exercises the happy path. The battery: valid PHPUnit-shaped junit producing byte-identical output to master, empty file, an attribute poisoned with a raw 0x80 byte matching the April incident, XML with no testsuite, missing failures or errors attributes, tests="0", a root testsuite file, and full report.php runs for the missing, unreadable, poisoned, and valid cases with exit codes checked. Each failure fixture was also run through master's code to confirm the described before behavior.
One related gap stays out of scope: in the wordpress-develop reporting workflow, later small PHPUnit invocations overwrite the same junit.xml target, so the uploaded result can reflect only the final group. That is core workflow territory and worth its own issue.
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code and Codex
Used for: Investigation, implementation review, edge-case testing, and PR wording. I reviewed the reasoning and test results, and I take responsibility for the contribution.