fix: CI smoke matrix routing - #1284
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughProduct smoke jobs now parse ChangesProduct smoke matrix selection
Model route retry cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: 🔵 Low · up to This PR changes CI smoke-matrix routing, but its regression test does not prove that each smoke ID remains attached to the intended job, so an incorrect routing change could pass CI while exercising the wrong coverage. The PR is mergeable with explicit owner awareness or follow-up to strengthen that assertion. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/tests/test_ci_lane_workflows.py`:
- Around line 204-220: Update
test_product_smoke_jobs_parse_formatted_matrix_json to verify each smoke ID
within its intended job block in ci-product-smoke-slice.yml, rather than
searching the entire workflow text. Parse or scope the workflow so the expected
job-to-ID mapping is enforced, while retaining the negative assertion that
rejects the obsolete direct inputs.smoke_matrix lookup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b7433e2f-3492-4670-8c6b-84af03304b1d
📒 Files selected for processing (2)
.github/workflows/ci-product-smoke-slice.ymlscripts/tests/test_ci_lane_workflows.py
| def test_product_smoke_jobs_parse_formatted_matrix_json(self) -> None: | ||
| workflow = self.workflow("ci-product-smoke-slice.yml") | ||
| for smoke_id in ( | ||
| "core", | ||
| "core-cuda", | ||
| "two-node-client", | ||
| "two-node-split", | ||
| "metal-model-load", | ||
| "model-download", | ||
| ): | ||
| with self.subTest(smoke_id=smoke_id): | ||
| self.assertIn( | ||
| f"contains(fromJson(inputs.smoke_matrix).*.id, '{smoke_id}')", | ||
| workflow, | ||
| ) | ||
| self.assertNotIn("contains(inputs.smoke_matrix,", workflow) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert each smoke ID on its intended job.
The test checks each expression anywhere in ci-product-smoke-slice.yml. It can pass when two IDs are attached to the wrong jobs. Scope each assertion to the matching job block, or parse the workflow and assert the job-to-ID mapping. Keep the negative assertion for the obsolete direct lookup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/tests/test_ci_lane_workflows.py` around lines 204 - 220, Update
test_product_smoke_jobs_parse_formatted_matrix_json to verify each smoke ID
within its intended job block in ci-product-smoke-slice.yml, rather than
searching the entire workflow text. Parse or scope the workflow so the expected
job-to-ID mapping is enforced, while retaining the negative assertion that
rejects the obsolete direct inputs.smoke_matrix lookup.
883901a to
ed9bc0c
Compare
1 participant
Summary by CodeRabbit
Bug Fixes
Tests