Add XML coverage support in multiple-files mode#248
Conversation
chore: setup tests for multi line with xml
Adds XML coverage file support alongside existing TXT in multiple-files input. Auto-detects format by .xml extension, adds safety checks for XML coverage structure, and includes tests and CI workflow. Co-Authored-By: Eric Hayes <hayes0724@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Added auto-detection for XML coverage files by `.xml` extension. - Enabled mixing of TXT and XML formats in multi-file reports, beneficial for monorepos. - Implemented a safety check for XML coverage structure to prevent runtime errors. - Removed obsolete CI workflow for multiline XML coverage. Co-authored-by: Eric Hayes <hayes0724@users.noreply.github.com>
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
WalkthroughThis change adds XML coverage file support to the multiple-files mode of Pytest Coverage Comment. Previously, the multiple-files feature only handled text-based coverage reports. The implementation now auto-detects XML coverage files by their Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds support for XML coverage files in the multiple-files input mode, enabling monorepos to mix TXT and XML coverage formats in a single comment. The feature auto-detects XML coverage files by their .xml extension and routes them to the appropriate parser. The implementation also includes a new test file to verify the mixed-format functionality.
Changes:
- Added XML coverage support in
multiple-filesmode with auto-detection by file extension - Extended
getMultipleReportfunction to handle both TXT and XML coverage formats - Added unit test for mixed TXT/XML coverage reporting
- Updated documentation to reflect new capability
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/multiple-files.test.js | New test file verifying mixed TXT/XML coverage in multi-file mode |
| src/multiFiles.js | Core implementation: auto-detects XML coverage by extension and routes to appropriate parser |
| package.json | Version bump to 1.3.0 and added test script |
| dist/index.js | Built distribution file with same changes as src/multiFiles.js |
| action.yml | Updated documentation to explain TXT/XML support in multiple-files input |
| CHANGELOG.md | Added release notes for version 1.3.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const output = newOptions.covXmlFile | ||
| ? getCoverageXmlReport(newOptions) | ||
| : getCoverageReport(newOptions); | ||
| core.setOutput('coverageHtml', output.html); |
There was a problem hiding this comment.
The getCoverageXmlReport function can return null (when parsedXml is invalid) or an empty string (on error), which will cause a runtime error when accessing output.html on line 86. The getCoverageReport function always returns a valid object with html, coverage, color, and warnings properties, but getCoverageXmlReport does not follow the same pattern. Either getCoverageXmlReport should be updated to always return a valid object (like getCoverageReport does), or add a null check here before accessing output.html.
| core.setOutput('coverageHtml', output.html); | |
| const coverageHtml = | |
| output && typeof output === 'object' && 'html' in output | |
| ? output.html | |
| : ''; | |
| core.setOutput('coverageHtml', coverageHtml); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/multiple-files.test.js (1)
36-57: Test assertions don't verify per-source correctness.The test only checks string presence across the whole table. It cannot distinguish whether TXT coverage was parsed as TXT and XML as XML. Consider adding at least one assertion that confirms the two rows are independently populated (e.g., count of
:zzz:occurrences equals 2, one per row).assert.ok(table.includes(':stopwatch:'), 'Time emoji should be present'); + + // Both rows should have JUnit summary columns + const zzz = (table.match(/:zzz:/g) || []).length; + assert.strictEqual(zzz, 2, 'Each row should have its own :zzz: (skipped) column');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/multiple-files.test.js` around lines 36 - 57, The test's assertions only check global string presence and don't ensure per-source rows are populated independently; update the test that calls getMultipleReport to verify per-row correctness by asserting counts and row-specific content (e.g., assert that occurrences of ':zzz:', ':x:', ':fire:', and ':stopwatch:' each equal 2 to ensure both TXT and XML rows include those columns, and add assertions that the 'TXT Title' row contains TXT-specific values and the 'XML Title' row contains XML-specific values), referencing getMultipleReport, 'TXT Title', 'XML Title', and the emoji strings to locate where to add these checks.package.json (1)
29-30:npm run alldoes not execute the new test suite.The
"all"script runslint && format && buildbut nottest. Runningnpm run allin CI will not catch test failures.💡 Suggested fix
- "all": "npm run lint && npm run format && npm run build", + "all": "npm run lint && npm run format && npm run build && npm test",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 29 - 30, Update the package.json "all" script to include running the test suite so CI runs tests; locate the "all" script entry (currently "all": "npm run lint && npm run format && npm run build") and append or insert the test step (invoke the existing "test" script name) so it executes lint, format, build, and test in sequence.src/multiFiles.js (1)
43-43:hasXmlReportsis now ambiguous — rename tohasJunitReports.Before this PR, "XML" meant JUnit XML. Now there are two XML types (
covXmlFilefor coverage XML,xmlFilefor JUnit XML). The variablehasXmlReportschecksl.xmlFile(JUnit XML), but its name now reads as "has XML coverage reports", which is the opposite of its meaning.- const hasXmlReports = lineReports.some((l) => l.xmlFile); + const hasJunitReports = lineReports.some((l) => l.xmlFile);(And update the two uses at lines 50 and 104.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/multiFiles.js` at line 43, Rename the ambiguous variable hasXmlReports to hasJunitReports to reflect that it checks JUnit XML (lineReports.some(l => l.xmlFile)) and not coverage XML (covXmlFile); update the variable declaration (currently const hasXmlReports = lineReports.some((l) => l.xmlFile)) and replace all its uses (the two places that reference hasXmlReports later in this file) to use hasJunitReports, and ensure no other logic is changed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/multiFiles.js`:
- Around line 83-86: The code assumes output.html exists but
getCoverageXmlReport can return null or empty string; update the block that
calls getCoverageXmlReport/getCoverageReport (the ternary producing output) to
handle falsy returns from getCoverageXmlReport: call the chosen function into a
local variable (e.g., output), then check that output is an object with an html
property before calling core.setOutput('coverageHtml', output.html), otherwise
provide a safe fallback (empty string or result from getCoverageReport) or throw
a clear error; reference getCoverageXmlReport, getCoverageReport, newOptions and
core.setOutput to locate where to add the null/empty guard.
- Around line 78-80: The current branch skips calling core.setOutput('warnings',
report.warnings) when internalOptions.covXmlFile is truthy, causing the warnings
output to be unset; ensure warnings is always emitted by setting
core.setOutput('warnings', String(report.warnings ?? 0)) (or explicitly '0')
either before/after the covXmlFile conditional or inside the covXmlFile branch
so that core.setOutput('warnings', ...) is invoked in all cases and downstream
consumers receive a numeric string.
---
Nitpick comments:
In `@package.json`:
- Around line 29-30: Update the package.json "all" script to include running the
test suite so CI runs tests; locate the "all" script entry (currently "all":
"npm run lint && npm run format && npm run build") and append or insert the test
step (invoke the existing "test" script name) so it executes lint, format,
build, and test in sequence.
In `@src/multiFiles.js`:
- Line 43: Rename the ambiguous variable hasXmlReports to hasJunitReports to
reflect that it checks JUnit XML (lineReports.some(l => l.xmlFile)) and not
coverage XML (covXmlFile); update the variable declaration (currently const
hasXmlReports = lineReports.some((l) => l.xmlFile)) and replace all its uses
(the two places that reference hasXmlReports later in this file) to use
hasJunitReports, and ensure no other logic is changed.
In `@test/multiple-files.test.js`:
- Around line 36-57: The test's assertions only check global string presence and
don't ensure per-source rows are populated independently; update the test that
calls getMultipleReport to verify per-row correctness by asserting counts and
row-specific content (e.g., assert that occurrences of ':zzz:', ':x:', ':fire:',
and ':stopwatch:' each equal 2 to ensure both TXT and XML rows include those
columns, and add assertions that the 'TXT Title' row contains TXT-specific
values and the 'XML Title' row contains XML-specific values), referencing
getMultipleReport, 'TXT Title', 'XML Title', and the emoji strings to locate
where to add these checks.
| if (!internalOptions.covXmlFile) { | ||
| core.setOutput('warnings', report.warnings); | ||
| } |
There was a problem hiding this comment.
warnings output is silently unset when the first multiple-files entry uses XML coverage.
When covXmlFile is active for the first entry, core.setOutput('warnings', ...) is never called. Users referencing steps.<id>.outputs.warnings will receive an empty string instead of '0', which may break downstream numeric comparisons. Consider explicitly defaulting it:
core.setOutput('color', report.color);
if (!internalOptions.covXmlFile) {
core.setOutput('warnings', report.warnings);
+ } else {
+ core.setOutput('warnings', '0');
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/multiFiles.js` around lines 78 - 80, The current branch skips calling
core.setOutput('warnings', report.warnings) when internalOptions.covXmlFile is
truthy, causing the warnings output to be unset; ensure warnings is always
emitted by setting core.setOutput('warnings', String(report.warnings ?? 0)) (or
explicitly '0') either before/after the covXmlFile conditional or inside the
covXmlFile branch so that core.setOutput('warnings', ...) is invoked in all
cases and downstream consumers receive a numeric string.
| const output = newOptions.covXmlFile | ||
| ? getCoverageXmlReport(newOptions) | ||
| : getCoverageReport(newOptions); | ||
| core.setOutput('coverageHtml', output.html); |
There was a problem hiding this comment.
Potential TypeError if getCoverageXmlReport returns null or '' on the second call.
getCoverageXmlReport can return null (invalid file) or '' (catch path). Unlike getCoverageReport, which always returns an object, the XML path has no null guard before accessing .html on line 86.
While the second call is likely to succeed (first call already validated the file, only commit changes), the asymmetry is a risk:
- core.setOutput('coverageHtml', output.html);
+ core.setOutput('coverageHtml', (output && output.html) || '');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/multiFiles.js` around lines 83 - 86, The code assumes output.html exists
but getCoverageXmlReport can return null or empty string; update the block that
calls getCoverageXmlReport/getCoverageReport (the ternary producing output) to
handle falsy returns from getCoverageXmlReport: call the chosen function into a
local variable (e.g., output), then check that output is an object with an html
property before calling core.setOutput('coverageHtml', output.html), otherwise
provide a safe fallback (empty string or result from getCoverageReport) or throw
a clear error; reference getCoverageXmlReport, getCoverageReport, newOptions and
core.setOutput to locate where to add the null/empty guard.
Summary
multiple-filesinput, auto-detected by.xmlextensionCloses #200
Test plan
npm test— unit tests for mixed TXT/XML multi-file coveragenpm run all— lint, format, build pass🤖 Generated with Claude Code