Conversation
WalkthroughA new pytest marker Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 Tip You can generate walkthrough in a markdown collapsible section to save space.Enable the |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/support_matrix/test_pr_support_matrix.py (1)
56-58: Consider logging when a (system, backend) pair has no database.When
_latest_versionreturnsNone, the combination is silently excluded from the test matrix. This is reasonable since not all backends may have databases for all systems, but adding a debug-level log could help developers understand test coverage gaps.🔧 Optional: Add logging for skipped combinations
+import logging + +logger = logging.getLogger(__name__) + def _build_param_grid() -> list[pytest.param]: """Build a flat list of pytest params for every valid (model, system, backend, version) combo.""" params: list[pytest.param] = [] for model in PR_MODELS: short_model = model.rsplit("/", 1)[-1] for system in PR_SYSTEMS: for backend in PR_BACKENDS: version = _latest_version(system, backend) if version is None: + logger.debug("No database version for %s/%s, skipping", system, backend) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/support_matrix/test_pr_support_matrix.py` around lines 56 - 58, When iterating combinations, add a debug-level log when _latest_version(system, backend) returns None so skipped (system, backend) pairs are visible; inside the block where you currently do "if version is None: continue" call the test logger (e.g. logger.debug or logging.getLogger(...).debug) with a concise message including system and backend identifiers and that no database/version was found, then continue. Ensure you reference the same variables (_latest_version, version, system, backend) so the log is added next to the existing check without changing control flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/support_matrix/test_pr_support_matrix.py`:
- Around line 56-58: When iterating combinations, add a debug-level log when
_latest_version(system, backend) returns None so skipped (system, backend) pairs
are visible; inside the block where you currently do "if version is None:
continue" call the test logger (e.g. logger.debug or
logging.getLogger(...).debug) with a concise message including system and
backend identifiers and that no database/version was found, then continue.
Ensure you reference the same variables (_latest_version, version, system,
backend) so the log is added next to the existing check without changing control
flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e24489af-5b1b-4d78-99e9-1308a228268a
📒 Files selected for processing (2)
pytest.initests/e2e/support_matrix/test_pr_support_matrix.py
Add a per-PR test for support matrix.
Assumptions
support_matrix.csvdoes not support, will skip it.Models Under Test
Perf