infra: Scope incremental cascade per parameter set#5086
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors pytest incremental failure tracking to store the last failing item per parametrization key (derived from ChangesIncremental test failure tracking per parameter identifier
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
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 |
Review Summary by QodoScope incremental cascade per parameter set
WalkthroughsDescription• Scope incremental cascade per parameter set to prevent cross-contamination • Store _previousfailed as dictionary keyed by parameter values • Extract parameter key from test item name using regex pattern • Prevent failures in one parametrized test from affecting others Diagramflowchart LR
A["Parametrized Incremental Test"] -->|Extract param key| B["_get_incremental_param_key"]
B -->|Return param value| C["Dictionary key"]
D["Test Failure"] -->|Store with param key| E["_previousfailed dict"]
E -->|Lookup by param key| F["Independent cascade per param"]
F -->|No cross-contamination| G["Other param sets unaffected"]
File Changes1. conftest.py
|
Code Review by Qodo
1. _get_incremental_param_key in conftest
|
|
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
the
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
added
size/XS
tox:verify-tc-requirement-polarion:passed
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Anatw
force-pushed
the
fix_incremental_implementation
branch
from
fdd7e83 to
42617c3
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5086 +/- ##
==========================================
- Coverage 98.67% 98.66% -0.02%
==========================================
Files 25 25
Lines 2487 2463 -24
==========================================
- Hits 2454 2430 -24
Misses 33 33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
/build-and-push-container |
Contributor
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5086 published |
openshift-virtualization-qe-bot-4
added
lgtm-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
Contributor
Author
|
/verified Verified locally on bm03-tlv2 Without this PR fix: With the current PRs fix: Test regression - class mark as incremental with no parametrization has no issues: Incremental with class parametrization: |
Contributor
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: |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
PR=5086
REPO="RedHatQE/openshift-virtualization-tests"
echo "## PR metadata"
gh pr view "$PR" --repo "$REPO" --json headRefOid,baseRefOid,files | jq -r '.'
echo "## PR diff"
gh pr diff "$PR" --repo "$REPO" --patch > /tmp/pr5086.patch
cat /tmp/pr5086.patch
echo "## first changed line in first file"
python3 - <<'PY'
from pathlib import Path
patch = Path('/tmp/pr5086.patch').read_text()
current_file = None
for line in patch.splitlines():
if line.startswith('diff --git '):
current_file = line.split(' b/', 1)[-1]
if line.startswith('@@') and current_file:
plus = line.split(' +', 1)[1].split(' ', 1)[0]
start = int(plus.split(',')[0])
print(f"file={current_file} line={start}")
break
PY
echo "## smoke marker files"
rg -l '`@pytest`\.mark\.smoke' tests/ || true
echo "## gating marker files"
rg -l '`@pytest`\.mark\.gating' tests/ || trueLength of output: 6371 🏁 Script executed: #!/bin/bash
set -euo pipefail
PR=5086
REPO="RedHatQE/openshift-virtualization-tests"
COMMIT_ID="0fa215343a44fd51739f051301611352e7e69168"
PATH_IN_PR="conftest.py"
LINE=676
BODY="**Test Execution Plan**
- **Run smoke tests: False** — Smoke-marked files (tests/virt/node/general/test_container_disk_vm.py, etc.) do not use \`pytest.mark.incremental\`; the modified hooks in \`pytest_runtest_makereport\` / \`pytest_runtest_setup\` only alter behavior for incremental-marked tests, so no smoke test traces to the changed code path.
- **Run gating tests: False** — Gating-marked files do not use \`pytest.mark.incremental\`; same reasoning as above.
**Affected tests to run**
- \`tests/network/l2_bridge/migration_stuntime/test_migration_stuntime.py\` \`(tier3, incremental, parametrized ipv4/ipv6)\` — primary target: validates that \`pytest_runtest_makereport\` → \`item.callspec.id\` key scoping prevents ipv4 failure cascading into ipv6.
- \`tests/network/localnet/migration_stuntime/test_migration_stuntime.py\` \`(tier3, incremental, parametrized ipv4/ipv6)\` — same parametrized incremental cascade path for localnet.
- \`tests/network/macspoof/test_macspoof.py\` \`(tier2, incremental, non-parametrized)\` — regression check: non-parametrized incremental classes fall back to \`param_key=\"\"\` and must preserve prior cascade behavior unchanged.
No hardware-specific fixture changes detected; no SR-IOV/GPU/DPDK split required."
payload_file="$(mktemp)"
jq -n \
--arg body "$BODY" \
--arg commit_id "$COMMIT_ID" \
--arg path "$PATH_IN_PR" \
--argjson line "$LINE" \
'{body:$body, commit_id:$commit_id, path:$path, side:"RIGHT", line:$line}' > "$payload_file"
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/$REPO/pulls/$PR/comments" \
--input "$payload_file" | jq -r '{id, path, line, html_url}'Length of output: 2071 🧠 Learnings used |
servolkov
approved these changes
Jun 3, 2026
Contributor
Author
|
/check-can-merge |
1 similar comment
Contributor
Author
|
/check-can-merge |
Contributor
|
/lgtm |
vsibirsk
approved these changes
Jun 4, 2026
This was referenced Jun 4, 2026
OhadRevah
pushed a commit
to OhadRevah/openshift-virtualization-tests
that referenced
this pull request
Jun 10, 2026
##### What this PR does / why we need it: When an incremental class is parametrized at class level, a failure in one parameter set (e.g. ipv4) should not cascade into another (e.g. ipv6) since they are independent test cycles with fresh fixtures. This was discovered while working on PR RedHatQE#5026 — adding `@pytest.mark.jira("CNV-88755", run=False)` to a single test caused 6 xfails instead of just 2 skips, because `_previousfailed` is stored on the class node which is shared across all parametrize combinations. ##### Which issue(s) this PR fixes: ##### Special notes for reviewer: - Non-parametrized incremental classes are unaffected — all their tests share the same key so behavior is identical to before. - Currently two incremental classes use class-level parametrize: - `tests/network/l2_bridge/migration_stuntime/test_migration_stuntime.py` - `tests/network/localnet/migration_stuntime/test_migration_stuntime.py` ##### jira-ticket: <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Improved incremental test failure tracking for parametrized tests, so failures are tracked per-parameter set—leading to clearer xfail attribution and more reliable test outcomes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Anat Wax <awax@redhat.com>
rnetser
pushed a commit
to rnetser/openshift-virtualization-tests
that referenced
this pull request
Jun 13, 2026
##### What this PR does / why we need it: When an incremental class is parametrized at class level, a failure in one parameter set (e.g. ipv4) should not cascade into another (e.g. ipv6) since they are independent test cycles with fresh fixtures. This was discovered while working on PR RedHatQE#5026 — adding `@pytest.mark.jira("CNV-88755", run=False)` to a single test caused 6 xfails instead of just 2 skips, because `_previousfailed` is stored on the class node which is shared across all parametrize combinations. ##### Which issue(s) this PR fixes: ##### Special notes for reviewer: - Non-parametrized incremental classes are unaffected — all their tests share the same key so behavior is identical to before. - Currently two incremental classes use class-level parametrize: - `tests/network/l2_bridge/migration_stuntime/test_migration_stuntime.py` - `tests/network/localnet/migration_stuntime/test_migration_stuntime.py` ##### jira-ticket: <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Improved incremental test failure tracking for parametrized tests, so failures are tracked per-parameter set—leading to clearer xfail attribution and more reliable test outcomes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Anat Wax <awax@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
When an incremental class is parametrized at class level, a failure in one parameter set (e.g. ipv4) should not cascade into another (e.g. ipv6) since they are independent test cycles with fresh fixtures.
This was discovered while working on PR #5026 — adding
@pytest.mark.jira("CNV-88755", run=False)to a single test caused 6 xfails instead of just 2 skips, because_previousfailedis stored on the class node which is shared across all parametrize combinations.Which issue(s) this PR fixes:
Special notes for reviewer:
tests/network/l2_bridge/migration_stuntime/test_migration_stuntime.pytests/network/localnet/migration_stuntime/test_migration_stuntime.pyjira-ticket:
Summary by CodeRabbit