Add warning for invalid nunit xml files #286
Conversation
WalkthroughThe changes in this pull request enhance error handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ResultsCheck
participant FileParser
participant Logger
User->>ResultsCheck: Initiate createCheck
ResultsCheck->>FileParser: Parse results files
alt Parsing successful
FileParser-->>ResultsCheck: Return parsed data
else Parsing error
FileParser-->>ResultsCheck: Return error
ResultsCheck->>Logger: Log warning about parsing error
end
ResultsCheck-->>User: Return summary of results
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
GabLeRoux
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/model/results-check.ts (1)
25-31: Consider enhancing error reporting and tracking.While the current implementation catches errors, it could be improved to provide better visibility into parsing issues:
- Track the number of failed files
- Include parsing failures in the final summary
- Distinguish between different types of errors (invalid XML vs non-NUnit format)
const runs: RunMeta[] = []; +const parsingErrors: Array<{file: string, error: string}> = []; const files = fs.readdirSync(artifactsPath); await Promise.all( files.map(async filepath => { if (!filepath.endsWith('.xml')) return; core.info(`Processing file ${filepath}...`); try { const fileData = await ResultsParser.parseResults(path.join(artifactsPath, filepath)); core.info(fileData.summary); runs.push(fileData); } catch (error: any) { core.warning(`Failed to parse ${filepath}: ${error.message}`); + parsingErrors.push({ file: filepath, error: error.message }); } }), ); +// Add parsing errors to the summary if any occurred +if (parsingErrors.length > 0) { + core.warning(`Failed to parse ${parsingErrors.length} file(s)`); + runSummary.addParsingErrors(parsingErrors); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (1)
src/model/results-check.ts(1 hunks)
🔇 Additional comments (2)
src/model/results-check.ts (2)
25-31: LGTM! Error handling implementation looks good.
The try-catch block appropriately handles parsing failures and allows processing to continue with other files.
25-31: Verify the impact on test results reporting.
The error handling might affect how test results are reported to GitHub Checks API. Let's verify the implementation of ResultsParser to ensure it properly distinguishes between different error types.
✅ Verification successful
Let me gather more information about the ResultsParser implementation and error handling.
Based on the implementation details and tests, I can now provide the final response:
Error handling in ResultsParser is robust and properly integrated with GitHub Checks API
The error handling implementation is well-designed and properly handles test results reporting:
- File parsing errors are caught and logged as warnings, allowing other test files to be processed
- Test failures are properly captured with detailed stack traces and console output
- Failed tests are correctly converted to GitHub Check annotations with proper file paths and line numbers
- The implementation includes comprehensive test coverage for various error scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ResultsParser implementation and error handling
ast-grep --pattern 'class ResultsParser {
$$$
parseResults($$$) {
$$$
}
$$$
}'
# Look for existing error handling in test files
rg -g "*.test.ts" "ResultsParser.parseResults"
Length of output: 494
Script:
#!/bin/bash
# Get the implementation of ResultsParser class
cat src/model/results-parser.ts
# Look for test files content to understand error scenarios
cat src/model/results-parser.test.ts
# Check if there are any GitHub Checks API related files
fd "check" --type f --exec cat {}
Length of output: 20115
|
|
Co-authored-by: Koji Hasegawa <[email protected]>
There was a problem hiding this comment.
LGTM!
And succeeds in re-run workflow.
https://github.com/nowsprinting/test-helper/actions/runs/11748649133
Closes #285
This pull request includes a change to the
src/model/results-check.tsfile to improve error handling when parsing XML files.Error handling improvement:
src/model/results-check.ts: Added atry-catchblock to handle errors during the parsing of XML files, logging a warning message if parsing fails.Summary by CodeRabbit
These changes enhance the reliability of file processing, ensuring smoother operation and better user experience.