fix: improve smoke test conftest-hierarchy tracing in CodeRabbit instructions#5096
Conversation
…ructions Add two safeguards to step 7 of the smoke/gating analysis: 1. VERIFY guard: require rg to return actual file paths before concluding 'Run smoke tests: False'. 2. conftest hierarchy check: for each smoke-marked file, scan parent conftest.py files up to the repo root for imports of modified utilities/libs symbols, since a smoke test can be transitively affected via conftest even when the test file itself has no direct import. Fixes a gap discovered in PR RedHatQE#4954 where smoke tests were incorrectly marked as unaffected. Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: rnetser <rnetser@redhat.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
More reviews will be available in 41 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe GitHub Actions workflow template for CodeRabbit test instructions is updated with more explicit smoke test decision verification. Lines 82–92 add instructions requiring validation that the smoke marker search command returns actual file paths before concluding ChangesWorkflow Smoke Test Decision Enhancement
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
|
review and take #4954 (comment) conversation into consideration. does it answer what is needed? |
![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 @.github/workflows/request-coderabbit-test-instructions.yml:
- Around line 83-89: The current rule narrows "Run smoke tests: True" to only
smoke-marked files found in the affected set from steps 6b-6e, but also
describes a separate hierarchical path where a conftest.py (and autouse
fixtures) can make a smoke test affected; fix by merging these paths into a
single consistent rule: treat a smoke test as affected if either (A) the test
file itself is in the affected set from 6b-6e OR (B) any conftest.py in its
parent-directory hierarchy imports or uses a transitively modified symbol
(including autouse fixtures); update the wording to explicitly state the union
(OR) logic and include the terms "Run smoke tests: True", "conftest.py", and
"autouse" so consumers apply both checks when determining affected smoke tests.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 37e66575-9d5b-415e-b641-316dcf433065
📒 Files selected for processing (1)
.github/workflows/request-coderabbit-test-instructions.yml
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-05-19T10:17:37.060Z
Learnt from: Anatw
Repo: RedHatQE/openshift-virtualization-tests PR: 4833
File: tests/network/localnet/migration_stuntime/libstuntime.py:13-13
Timestamp: 2026-05-19T10:17:37.060Z
Learning: In RedHatQE/openshift-virtualization-tests, when the PR template validation rule is triggered, avoid posting the PR template violation comment if the PR description already contains the required sections with meaningful content:
- `##### What this PR does / why we need it:`
- `##### Which issue(s) this PR fixes:`
- `##### Special notes for reviewer:`
- `##### jira-ticket:`
Because the current implementation can use a broad `**` path glob and re-run the check per diff context/file, reviewers/automation should verify the actual PR description content before flagging it as a violation or duplicating the comment.
Applied to files:
.github/workflows/request-coderabbit-test-instructions.yml
📚 Learning: 2026-05-20T11:18:31.677Z
Learnt from: Anatw
Repo: RedHatQE/openshift-virtualization-tests PR: 4867
File: tests/network/libs/stuntime.py:1-1
Timestamp: 2026-05-20T11:18:31.677Z
Learning: In RedHatQE/openshift-virtualization-tests, make the PR template validation (HIGH-severity) heuristic more conservative: if the author explicitly rebuts the “missing template section” finding and confirms the required sections are present with meaningful content, treat the original finding as a false positive and do not re-raise it. Only raise the HIGH-severity flag when a required PR section header (e.g., "##### What this PR does / why we need it:") is clearly absent or its content can be verified as empty/placeholder-only.
Applied to files:
.github/workflows/request-coderabbit-test-instructions.yml
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch PR `#5096` details and diff
gh pr view 5096 --repo RedHatQE/openshift-virtualization-tests --json title,body,files,additions,deletions
# Also fetch the PR `#4954` discussion comment
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/comments/3334667036 2>/dev/null || \
gh api repos/RedHatQE/openshift-virtualization-tests/issues/comments/3334667036 2>/dev/null || \
echo "Trying review comment API..."
gh api "repos/RedHatQE/openshift-virtualization-tests/pulls/4954/comments" --jq '.[] | select(.id == 3334667036) | {id, body, path, line, diff_hunk}' 2>/dev/null || trueLength of output: 11140 🏁 Script executed: #!/bin/bash
# Find the YAML file modified in the PR
fd -e yml -e yaml . .github/ | head -20
git log --oneline -5
git diff HEAD~1 HEAD -- '*.yml' '*.yaml' 2>/dev/null | head -300Length of output: 887 🏁 Script executed: #!/bin/bash
# Get the full content of the modified file
cat .github/workflows/request-coderabbit-test-instructions.ymlLength of output: 10392
What went wrong in PR
|
| Addition | Severity | Why it helps |
|---|---|---|
VERIFY guard — require rg to return actual paths before concluding False |
HIGH | Prevents short-circuiting on empty output |
conftest hierarchy check — scan dirname(smoke_file)/conftest.py up to root for modified symbol imports |
CRITICAL | This is exactly the path that was missed in #4954 |
| autouse fixture check — if an autouse fixture in any conftest calls a modified function, all tests below are affected | HIGH | Covers the implicit autouse dependency pattern common in this repo |
One minor observation
Step 7's conftest check currently says "imports of modified utilities/libs symbols." This is slightly narrower than fully transitive (a conftest importing a wrapper that calls the modified symbol). However, since Steps 6d–6e already expand the modified symbol set transitively before Step 7 runs, the conftest check will correctly operate on the already-expanded set. The logic chain holds.
Bottom line
The instructions in this PR would have caught the tests/storage/conftest.py → wait_for_hco_conditions path and flipped the smoke decision to True. The fix is well-targeted and sufficient for the reported case.
[approve]
Use OR logic: smoke test is affected if (A) file is in affected set from 6b-6e OR (B) conftest.py hierarchy imports modified symbols (including autouse fixtures). Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: rnetser <rnetser@redhat.com>
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
/verified (nothing to verify in this PR; approved by cr) |
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the diff of the PR to find the first changed line
gh pr diff 5096 --repo RedHatQE/openshift-virtualization-tests | head -60Length of output: 1971 🏁 Script executed: #!/bin/bash
# Get head commit SHA for the PR
gh pr view 5096 --repo RedHatQE/openshift-virtualization-tests --json headRefOid -q '.headRefOid'Length of output: 280 🏁 Script executed: #!/bin/bash
# Post inline review comment on first changed line of the PR
COMMENT_BODY="**Test Execution Plan**
**Run smoke tests: False** — This PR modifies only \`.github/workflows/request-coderabbit-test-instructions.yml\` (YAML workflow instruction text, +10/-0). No \`utilities/\`, \`libs/\`, test files, or conftest files were changed. No dependency path from any smoke-marked test to a modified symbol exists.
**Run gating tests: False** — Same reasoning; no test code, fixtures, or shared utilities modified.
**Affected tests to run:** None."
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
repos/RedHatQE/openshift-virtualization-tests/pulls/5096/comments \
-f body="$COMMENT_BODY" \
-f commit_id="ea003f6d076a6c3e71f38b056fa24535292a1a19" \
-f path=".github/workflows/request-coderabbit-test-instructions.yml" \
-f side="RIGHT" \
-F line=82Length of output: 4393 |
|
/approve |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published |
…ructions (RedHatQE#5096) ##### What this PR does / why we need it: Addresses the gap discovered in RedHatQE#4954 where smoke-marked tests were incorrectly concluded as unaffected by utility changes. ### Root cause Step 7 of the test execution plan instructions only checked whether a smoke-marked **test file** directly imported a modified symbol. It did not scan the `conftest.py` hierarchy of each smoke test for transitive imports or autouse fixtures — which is the actual dependency path in this repo. ### Changes Three additions to step 7: 1. **VERIFY guard** — require the `rg` command to return actual file paths before concluding `False` 2. **conftest hierarchy check** — for each smoke-marked file, scan parent conftest.py files up to the repo root for imports of modified utilities/libs symbols 3. **autouse fixture check** — if a conftest.py defines an autouse fixture that imports/calls a modified function, ALL tests in that directory and below are affected ### Example dependency path this fix catches ``` tests/storage/cdi_clone/test_clone.py (smoke-marked) → tests/storage/conftest.py (conftest in hierarchy) → from utilities.hco import wait_for_hco_conditions ← modified in PR RedHatQE#4954 ``` ##### Which issue(s) this PR fixes: Reported in: RedHatQE#4954 (CodeRabbit review comment) ##### Special notes for reviewer: Only changes a YAML workflow instruction text — no code changes. ##### jira-ticket: Assisted-by: Claude <noreply@anthropic.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Improved internal testing workflow verification procedures for development processes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: rnetser <rnetser@redhat.com>
6 participants
What this PR does / why we need it:
Addresses the gap discovered in #4954 where smoke-marked tests were incorrectly concluded as unaffected by utility changes.
Root cause
Step 7 of the test execution plan instructions only checked whether a smoke-marked test file directly imported a modified symbol. It did not scan the
conftest.pyhierarchy of each smoke test for transitive imports or autouse fixtures — which is the actual dependency path in this repo.Changes
Three additions to step 7:
rgcommand to return actual file paths before concludingFalseExample dependency path this fix catches
Which issue(s) this PR fixes:
Reported in: #4954 (CodeRabbit review comment)
Special notes for reviewer:
Only changes a YAML workflow instruction text — no code changes.
jira-ticket:
Assisted-by: Claude noreply@anthropic.com
Summary by CodeRabbit