net, libs: Refactor modules and add new lib helper#5072
Conversation
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? |
📝 WalkthroughWalkthroughPR consolidates test-local library imports to shared ChangesNetwork library consolidation and VLAN iterator
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes The PR consists primarily of repetitive import path migrations (13 files following an identical pattern) paired with one small new VLAN iterator feature and two straightforward type annotation updates. Imports are low-risk structural changes with no logic mutations; the iterator is modest in complexity (caching, normalization, yielding); type narrowing is a simple signature adjustment. Homogeneous refactoring across many files reduces per-file review effort. 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 |
|
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-2
requested review from
Anatw,
EdDev,
dshchedr,
nirdothan,
orelmisan,
rnetser,
vsibirsk and
yossisegev
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
azhivovk
force-pushed
the
net-refactor-shared-libs
branch
from
f44793f to
dd4f256
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
|
Clean rebase detected — no code changes compared to previous head ( |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
azhivovk
force-pushed
the
net-refactor-shared-libs
branch
from
dd4f256 to
f09904b
Compare
|
Change: rebase and fix checks errors |
openshift-virtualization-qe-bot-2
removed
the
changes-requested-coderabbitai[bot]
label
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
/verified |
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:
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: Before setting "Run smoke tests: True", you MUST use shell scripts to: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check smoke test usage of changed files/functions
echo "=== Smoke tests using nodenetworkconfigurationpolicy ==="
rg -l "smoke" tests/network/ --include="*.py" | head -20
echo "=== Files with smoke marker that import changed modules ==="
rg -l "`@pytest.mark.smoke`" tests/ --include="*.py" | xargs grep -l "nodenetworkconfigurationpolicy\|cluster_vlan_iterator\|wait_for_vmi_condition\|apimachinery" 2>/dev/null | head -20
echo "=== Smoke tests in network ==="
rg "`@pytest.mark.smoke`" tests/network/ -l --include="*.py" | head -20
echo "=== Check if tests/conftest.py next_vlan_index_number is used anywhere ==="
rg "next_vlan_index_number" tests/ --include="*.py" -n | head -20
echo "=== BGP tests using smoke ==="
rg "`@pytest.mark.smoke`" tests/network/bgp/ --include="*.py" -n | head -20
echo "=== l2_bridge tests using smoke ==="
rg "`@pytest.mark.smoke`" tests/network/l2_bridge/ --include="*.py" -n | head -20
echo "=== localnet tests using smoke ==="
rg "`@pytest.mark.smoke`" tests/network/localnet/ --include="*.py" -n | head -20Length of output: 4568 🏁 Script executed: #!/bin/bash
# Get the first changed file in the PR diff via GitHub API and find the first changed line
gh pr view 5072 --json files --jq '.files[0].path'
echo "---"
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5072/files --jq '.[0] | {path: .filename, patch: .patch}' | head -50Length of output: 467 🏁 Script executed: #!/bin/bash
# Find first file with an actual patch
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5072/files --jq '.[] | select(.patch != null) | {path: .filename, first_line: (.patch | split("\n")[0])}' | head -60Length of output: 1467 🏁 Script executed: #!/bin/bash
# Check smoke test usage with correct rg syntax
echo "=== Smoke tests in network dir ==="
rg "`@pytest.mark.smoke`" tests/network/ -l | head -20
echo "=== next_vlan_index_number usage ==="
rg "next_vlan_index_number" tests/ -n | head -20
echo "=== Smoke tests importing changed modules ==="
rg "`@pytest.mark.smoke`" tests/ -l | xargs grep -l "nodenetworkconfigurationpolicy\|cluster_vlan_iterator\|wait_for_vmi_condition\|apimachinery" 2>/dev/null | head -20
echo "=== vmspec wait_for_vmi usages ==="
rg "wait_for_vmi_condition" tests/ -n | head -20Length of output: 1189 🏁 Script executed: #!/bin/bash
# Get PR head SHA
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5072 --jq '.head.sha'Length of output: 248 🏁 Script executed: #!/bin/bash
BODY="**Test Execution Plan**
- **Run smoke tests: False** _(No smoke tests found in the network test suite; no dependency chain from \`@pytest.mark.smoke\` tests to any changed fixture or utility was detected.)_
**Changed files and affected tests:**
- \`tests/network/bgp/\` — conftest import path changed (\`libnncp\`); verify collection and fixture resolution:
- \`-m bgp\`
- \`tests/network/l2_bridge/\` — conftest import paths changed across all sub-packages; verify collection:
- \`-m l2_bridge\`
- \`tests/network/l2_bridge/nad_ref_change/\` — additionally uses \`wait_for_vmi_condition_status\` / \`wait_for_no_vmi_condition\` (type widened to \`VirtualMachine\`):
- \`tests/network/l2_bridge/nad_ref_change/\`
- \`tests/network/localnet/\` — conftest import paths changed; verify collection:
- \`-m localnet\`
- \`tests/conftest.py::next_vlan_index_number\` — new fixture, not yet consumed by any test; no execution required now."
gh api \
repos/RedHatQE/openshift-virtualization-tests/pulls/5072/comments \
-X POST \
-f commit_id="fcc50e711b6393bae1924f1d47fbb80f9f0cc080" \
-f path="libs/net/cluster.py" \
-f side="RIGHT" \
-F line=1 \
-f body="$BODY"Length of output: 4689 |
|
@azhivovk both the helper and the fixture are dead code right now, it contradicts the repo rule. I think we are in trouble here. |
|
@servolkov |
Yes, I read the description, I see. But I can only repeat myself - the code is dead. If fixture is used in that PR, what is the problem to introduce it there? |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4954. Overlapping filestests/network/localnet/liblocalnet.py |
|
@servolkov |
Sure. To avoid "dead code" PRs it must be there. |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4985. Overlapping filestests/conftest.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5039. Overlapping filestests/network/localnet/liblocalnet.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5026. Overlapping filestests/network/l2_bridge/migration_stuntime/conftest.py |
9 participants
What this PR does / why we need it:
Upcoming NAD live update metric test requires usage of existing network setup (NNCP, NAD).
This change promotes shared network infrastructure from test-specific locations to the shared libs layer so any test suite can use them without cross-package imports.
Which issue(s) this PR fixes: -
Special notes for reviewer:
Network tests which use existing VLAN fixtures will be adjusted to the new cluster.py VLAN helper in a follow-up, as this PR is planned to be backported to 4.22 and will affect network tests.
New VLAN fixture will be used in the upcoming NAD live update metric test.
Exception ticket to backport to 4.22: https://redhat.atlassian.net/browse/CNV-88886
jira-ticket: -
Summary by CodeRabbit
New Features
Chores