storage, tests, cdi_upload: Verify CDI upload in primary UDN namespace#5214
storage, tests, cdi_upload: Verify CDI upload in primary UDN namespace#5214Anatw wants to merge 1 commit into
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? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds UDN-backed CDI upload test coverage by introducing two pytest fixtures ( ChangesUDN-backed CDI Upload Test
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped 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
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
requested review from
acinko-rh,
josemacassan and
kgoldbla
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
removed
the
tox:verify-tc-requirement-polarion:running
|
D/S test |
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:failed
![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 `@tests/storage/cdi_upload/test_upload_virtctl.py`:
- Around line 418-423: The test function
`test_virtctl_image_upload_dv_in_pudn_namespace` declares `download_image` as a
function parameter but never uses its return value, only its side effects.
Remove `download_image` from the function parameters and add it to the existing
`@pytest.mark.usefixtures` decorator alongside `primary_udn_for_upload` to
explicitly declare the fixture dependency without creating an unused-argument
lint warning.
🪄 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: d928feea-57f4-4c4d-8b7e-48034824af0c
📒 Files selected for processing (2)
tests/storage/cdi_upload/conftest.pytests/storage/cdi_upload/test_upload_virtctl.py
Anatw
force-pushed
the
add_volume_upload_to_pudn_scenario
branch
from
629d196 to
b7e1dfb
Compare
openshift-virtualization-qe-bot
added
commented-coderabbitai[bot]
tox:verify-tc-requirement-polarion:running
|
/approve |
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5214 published |
|
/verified Tested on net-awax-422 openshift-virtualization-tests-runner/5690 |
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
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: 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 PR diff to identify first changed file and first changed line
gh pr view 5214 --repo RedHatQE/openshift-virtualization-tests --json files --jq '.files[].path'
# Check for smoke-marked tests in cdi_upload directory
rg -n '`@pytest.mark.smoke`' tests/storage/cdi_upload/ 2>/dev/null || echo "No smoke tests in cdi_upload"
# Check for gating-marked tests in cdi_upload directory
rg -n '`@pytest.mark.gating`' tests/storage/cdi_upload/ 2>/dev/null || echo "No gating tests in cdi_upload"
# Check all smoke test files in repo
rg -l '`@pytest.mark.smoke`' tests/ 2>/dev/null | head -20
# Check all gating test files in repo
rg -l '`@pytest.mark.gating`' tests/ 2>/dev/null | head -20
# Check if new fixtures in conftest are used anywhere else
rg -rn 'udn_namespace_for_dv_upload\|primary_udn_for_upload' tests/ 2>/dev/null
# Get the PR diff to find specific line numbers in the first changed file
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5214/files --jq '.[0] | {filename: .filename, patch: .patch}'Length of output: 4333 🏁 Script executed: #!/bin/bash
# Post inline review comment on first changed line of conftest.py
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5214/reviews \
--method POST \
--field event="COMMENT" \
--field body="" \
--field "comments[][path]=tests/storage/cdi_upload/conftest.py" \
--field "comments[][line]=10" \
--field "comments[][side]=RIGHT" \
--field "comments[][body]=**Test Execution Plan**
**Run smoke tests: False**
No traced path from the new non-autouse fixtures (\`udn_namespace_for_dv_upload\`, \`primary_udn_for_upload\`) to any smoke-marked test. The smoke test at \`test_upload_virtctl.py:154\` does not depend on the new fixtures.
**Run gating tests: False**
No traced path from the new fixtures to the gating-marked test at \`test_upload_virtctl.py:233\`. That test does not use \`udn_namespace_for_dv_upload\` or \`primary_udn_for_upload\`.
**Affected tests to run**
\`\`\`
tests/storage/cdi_upload/test_upload_virtctl.py::test_virtctl_image_upload_dv_in_pudn_namespace
\`\`\`
(new test, \`@pytest.mark.sno\`, validates CDI upload proxy reachability in a Primary UDN namespace — the scenario fixed by CNV-58018)"Length of output: 2932 |
|
/check-can-merge |
Add a regression test for disk image upload to a DataVolume in a PUDN namespace, after a bug (CNV-58018). Signed-off-by: Anat Wax <awax@redhat.com> Assisted-by: Claude <noreply@anthropic.com>
Anatw
force-pushed
the
add_volume_upload_to_pudn_scenario
branch
from
f935701 to
0682593
Compare
|
Change: rebase on main |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
D/S test |
|
/verified No code changes - #5214 (comment) |
|
Clean rebase detected — no code changes compared to previous head ( |
| yield dv | ||
|
|
||
|
|
||
| @pytest.fixture(scope="module") |
There was a problem hiding this comment.
What is the reason for setting udn_namespace_for_dv_upload and also primary_udn_for_upload module scope ?
If you only need them for a single test test_virtctl_image_upload_dv_in_pudn_namespace shouldn't you delete them at the end of the test?
There was a problem hiding this comment.
It was requested by Jenia - #5214 (comment)
11 participants
What this PR does / why we need it:
Add a regression test that uploads a disk image to a DataVolume in a PUDN namespace. This tests regression for bug CNV-58018 (CDI upload proxy could not reach the uploader pod in namespaces with a primary UDN).
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
NONE
Summary by CodeRabbit