test: use process restult scripts from itk repository - #113
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
⚠️ Not ready to approve
The updated ITK runner loses some important response-shape validation in nightly mode and the CI inline parser should handle malformed results more robustly.
Pull request overview
Updates the ITK test runner to rely on the canonical results-processing script from the a2a-itk repository (supporting the broader ITK consolidation effort), while removing the local results post-processor.
Changes:
- Nightly: persist
raw_results.jsonand invokea2a-itk/scripts/process_results.pyto produce the rolling history artifact. - CI: replace the removed
process_results.py ciinvocation with an inline Python summary/exit-code block. - Remove the repo-local
itk/process_results.pyand adjust.gitignore.
File summaries
| File | Description |
|---|---|
itk/run_itk.sh |
Switches nightly metrics processing to the upstream a2a-itk script and inlines CI result parsing/output. |
itk/process_results.py |
Removes the local ITK results post-processor in favor of the upstream script. |
.gitignore |
Adds itk/Cargo.lock to ignored artifacts. |
Review details
Comments suppressed due to low confidence (1)
itk/run_itk.sh:136
- In the CI inline Python,
results = data.get('results', {})is not type-checked before callingresults.items(). If the service returns a malformedresultsvalue, this will raise an unhandled exception/stack trace instead of a clear error message. Validate thatresultsis a JSON object and normalizepassedto a boolean before printing statuses.
all_passed = data.get('all_passed', False)
results = data.get('results', {})
for name, value in results.items():
passed = value.get('passed') if isinstance(value, dict) else value
status = 'PASSED' if passed else 'FAILED'
- Files reviewed: 2/3 changed files
- Comments generated: 1
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
✅ Ready to approve
The consolidation to the upstream nightly processor is consistent with the stated goal and the remaining issues are minor documentation/portability nits with straightforward fixes.
Note: this review does not count toward required approvals for merging.
Review details
Comments suppressed due to low confidence (2)
itk/run_itk.sh:131
- Same portability concern here: prefer
printfoverechowhen writing arbitrary JSON to a file.
echo "$RESPONSE" > raw_results.json
itk/run_itk.sh:140
- Using
echoto pipe JSON is not fully portable (someechoimplementations treat backslashes/flags specially).printfis safer for arbitrary JSON payloads.
echo "$RESPONSE" | python3 -c "
- Files reviewed: 2/3 changed files
- Comments generated: 2
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
| # Validate the ITK response shape BEFORE branching into nightly/CI post-processing. | ||
| # Guards against both branches silently accepting a malformed or error response: | ||
| # - CI mode: prevents an unhandled AttributeError inside the summariser. | ||
| # - Nightly mode: prevents the metrics processor from producing an empty/invalid | ||
| # history entry that would then be uploaded to the release asset. |
| # - CI mode: prevents an unhandled AttributeError inside the summariser. | ||
| # - Nightly mode: prevents the metrics processor from producing an empty/invalid | ||
| # history entry that would then be uploaded to the release asset. | ||
| echo "$RESPONSE" | python3 -c " |
Description
Uses process results script provided from itk repository instead of custom one - as part of ITK consolidation