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 |
Ports WordPress#339 (fixes upstream WordPress#311) into the fork, adapted to our terminus-fetched $logs_local paths. c65ae74 guarded *around* process_junit_xml() — a filesize() check before and an empty-return check after. The second guard was unreachable: on unparseable XML, simplexml_load_string() returns false and the function fatals at $xml->xpath() before it can return, which is exactly the uncaught Error every host hit in April (upstream WordPress#310, raw 0x80 bytes in the email test data). Verified against main: the poisoned fixture dies with "Call to a member function xpath() on bool", not our message. Guards now live in process_junit_xml() where the parse happens: - empty input aborts with a clear message - parse failure aborts and reports the first libxml error and line number, restoring the previous libxml handler state either way - missing tests/failures/errors counts abort, because a payload with a test count but no failure/error counts casts to zero downstream and renders as Passed — the actual WordPress#311 false positive - a root-level <testsuite> now parses (xpath drops the //testsuites// prefix); no-op for PHPUnit output, whose root is always <testsuites> report.php keeps the crash/OOM-specific message for the 0-byte case (our libaom SIGABRT signature, see BUGS-11823), adds is_readable(), and drops the now-dead post-call guard. Verified with a local fixture battery (no unit suite in this repo): valid PHPUnit XML produces byte-identical output to main; empty, 0x80-poisoned, no-testsuite, and missing-counts fixtures each exit 1 with a distinct message; tests="0" and root-<testsuite> pass through. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Reviewed and tested the failure paths locally on PHP 8.5, against both master and this branch.
Fixtures: a valid PHPUnit junit.xml, an empty file, an invalid-UTF-8 file matching the April #310 incident, a file with no , one missing the failures/errors counts, one with tests="0", and a root-level .
- Valid input produces byte-identical output to master. No happy-path regression.
- A missing failures/errors count serializes to empty on master and renders as a pass. This branch aborts instead.
- The #310 invalid-UTF-8 file kills master with a fatal (xpath() on false). This branch stops with the libxml parser error and line number.
- report.php now stops with a clear error when junit.xml is missing or unreadable.
Every failure path exits 1 with a readable message instead of uploading a bad result or crashing. Does what it set out to. LGTM.
AI Assisted used Claude Code running Opus 4.8 for spelling and double checking test results.
4 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.