Skip to content

Conversation

@hmeir
Copy link
Contributor

@hmeir hmeir commented Dec 7, 2025

Signed-off-by: Harel Meir [email protected]

Short description:

Bug fixed, we can remove the jira ref

More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:

https://issues.redhat.com/browse/CNV-71823

Summary by CodeRabbit

  • Tests
    • Removed an obsolete test fixture and associated conditional skip logic, simplifying test setup and reducing flaky/conditional test outcomes to improve reliability and maintainability.

✏️ Tip: You can customize this high-level summary in your review settings.

@openshift-virtualization-qe-bot

Report bugs in Issues

Welcome! 🎉

This pull request will be automatically processed with the following features:

🔄 Automatic Actions

  • Reviewer Assignment: Reviewers are automatically assigned based on the OWNERS file in the repository root
  • Size Labeling: PR size labels (XS, S, M, L, XL, XXL) are automatically applied based on changes
  • Issue Creation: A tracking issue is created for this PR and will be closed when the PR is merged or closed
  • Pre-commit Checks: pre-commit runs automatically if .pre-commit-config.yaml exists
  • Branch Labeling: Branch-specific labels are applied to track the target branch
  • Auto-verification: Auto-verified users have their PRs automatically marked as verified

📋 Available Commands

PR Status Management

  • /wip - Mark PR as work in progress (adds WIP: prefix to title)
  • /wip cancel - Remove work in progress status
  • /hold - Block PR merging (approvers only)
  • /hold cancel - Unblock PR merging
  • /verified - Mark PR as verified
  • /verified cancel - Remove verification status
  • /reprocess - Trigger complete PR workflow reprocessing (useful if webhook failed or configuration changed)

Review & Approval

  • /lgtm - Approve changes (looks good to me)
  • /approve - Approve PR (approvers only)
  • /automerge - Enable automatic merging when all requirements are met (maintainers and approvers only)
  • /assign-reviewers - Assign reviewers based on OWNERS file
  • /assign-reviewer @username - Assign specific reviewer
  • /check-can-merge - Check if PR meets merge requirements

Testing & Validation

  • /retest tox - Run Python test suite with tox
  • /retest build-container - Rebuild and test container image
  • /retest all - Run all available tests

Container Operations

  • /build-and-push-container - Build and push container image (tagged with PR number)
    • Supports additional build arguments: /build-and-push-container --build-arg KEY=value

Cherry-pick Operations

  • /cherry-pick <branch> - Schedule cherry-pick to target branch when PR is merged
    • Multiple branches: /cherry-pick branch1 branch2 branch3

Label Management

  • /<label-name> - Add a label to the PR
  • /<label-name> cancel - Remove a label from the PR

✅ Merge Requirements

This PR will be automatically approved when the following conditions are met:

  1. Approval: /approve from at least one approver
  2. LGTM Count: Minimum 2 /lgtm from reviewers
  3. Status Checks: All required status checks must pass
  4. No Blockers: No WIP, hold, or conflict labels
  5. Verified: PR must be marked as verified (if verification is enabled)

📊 Review Process

Approvers and Reviewers

Approvers:

  • dshchedr
  • myakove
  • rnetser
  • vsibirsk

Reviewers:

  • OhadRevah
  • RoniKishner
  • albarker-rh
  • dshchedr
  • hmeir
  • rlobillo
  • rnetser
  • vsibirsk
Available Labels
  • hold
  • verified
  • wip
  • lgtm
  • approve
  • automerge

💡 Tips

  • WIP Status: Use /wip when your PR is not ready for review
  • Verification: The verified label is automatically removed on each new commit
  • Cherry-picking: Cherry-pick labels are processed when the PR is merged
  • Container Builds: Container images are automatically tagged with the PR number
  • Permission Levels: Some commands require approver permissions
  • Auto-verified Users: Certain users have automatic verification and merge privileges

For more information, please refer to the project documentation or contact the maintainers.

@coderabbitai
Copy link

coderabbitai bot commented Dec 7, 2025

Walkthrough

Removed a JIRA-related pytest fixture and its conditional xfail handling: deleted the is_jira_71823_open fixture, removed HCO_BEARER_AUTH from imports, and removed the xfail branch in related_object_from_hco_status in tests/install_upgrade_operators/conftest.py.

Changes

Cohort / File(s) Summary
Test configuration cleanup
tests/install_upgrade_operators/conftest.py
Removed HCO_BEARER_AUTH import, deleted the is_jira_71823_open pytest fixture, and removed the conditional pytest.xfail branch inside related_object_from_hco_status that referenced the fixture and constant.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Single-file change with minor API/signature update and removal of a conditional.
  • Review attention points:
    • Ensure no other tests rely on the removed is_jira_71823_open fixture.
    • Confirm no remaining uses of HCO_BEARER_AUTH in the test suite that expect it to be available.

Possibly related PRs

Suggested labels

lgtm-openshift-virtualization-qe-bot

Suggested reviewers

  • dshchedr
  • rnetser
  • rlobillo
  • RoniKishner
  • OhadRevah

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: removing a Jira reference (CNV-71823) from the IUO test configuration, which matches the changeset content.
Description check ✅ Passed The description follows the required template structure with all sections present, though some sections lack detail. The jira-ticket field is properly filled with the relevant issue URL.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3ad50fd and 8a6432e.

📒 Files selected for processing (1)
  • tests/install_upgrade_operators/conftest.py (1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 2469
File: utilities/sanity.py:139-142
Timestamp: 2025-11-08T07:36:57.616Z
Learning: In the openshift-virtualization-tests repository, user rnetser prefers to keep refactoring PRs (like PR #2469) strictly focused on moving/organizing code into more granular modules without adding new functionality, error handling, or behavioral changes. Such improvements should be handled in separate PRs.
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 954
File: tests/storage/storage_migration/conftest.py:264-269
Timestamp: 2025-05-28T10:50:56.122Z
Learning: In the openshift-virtualization-tests codebase, cleanup pytest fixtures like `deleted_old_dvs_of_stopped_vms`, `deleted_completed_virt_launcher_source_pod`, and `deleted_old_dvs_of_online_vms` do not require yield statements. These fixtures perform cleanup operations and work correctly without yielding values.
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 2509
File: tests/virt/cluster/aaq/conftest.py:0-0
Timestamp: 2025-11-05T01:28:39.322Z
Learning: In pytest fixtures within the openshift-virtualization-tests repo, when using `yield from` to delegate to a generator function for setup/teardown (like `update_hco_memory_overcommit`), the delegated function should be a plain generator function without the `contextmanager` decorator. Using `contextmanager` causes a TypeError because it wraps the generator in a `_GeneratorContextManager` object, which is not iterable. The correct pattern is: remove `contextmanager` and use `yield from plain_generator_function(...)` in the fixture.
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 1932
File: tests/virt/node/migration_and_maintenance/conftest.py:65-72
Timestamp: 2025-09-29T20:33:51.007Z
Learning: In tests/virt/node/migration_and_maintenance/conftest.py, the added_vm_cpu_limit fixture doesn't require ResourceEditor as a context manager because it's the final test to modify the vm_for_multifd_test VM before teardown, so restoration of CPU limits is unnecessary overhead as confirmed by maintainer dshchedr.
📚 Learning: 2025-11-05T01:28:39.322Z
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 2509
File: tests/virt/cluster/aaq/conftest.py:0-0
Timestamp: 2025-11-05T01:28:39.322Z
Learning: In pytest fixtures within the openshift-virtualization-tests repo, when using `yield from` to delegate to a generator function for setup/teardown (like `update_hco_memory_overcommit`), the delegated function should be a plain generator function without the `contextmanager` decorator. Using `contextmanager` causes a TypeError because it wraps the generator in a `_GeneratorContextManager` object, which is not iterable. The correct pattern is: remove `contextmanager` and use `yield from plain_generator_function(...)` in the fixture.

Applied to files:

  • tests/install_upgrade_operators/conftest.py
📚 Learning: 2025-06-18T09:21:34.315Z
Learnt from: OhadRevah
Repo: RedHatQE/openshift-virtualization-tests PR: 1166
File: tests/observability/metrics/conftest.py:1065-1077
Timestamp: 2025-06-18T09:21:34.315Z
Learning: In tests/observability/metrics/conftest.py, when creating fixtures that modify shared Windows VM state (like changing nodeSelector), prefer using function scope rather than class scope to ensure ResourceEditor context managers properly restore the VM state after each test, maintaining test isolation while still reusing expensive Windows VM fixtures.

Applied to files:

  • tests/install_upgrade_operators/conftest.py
📚 Learning: 2025-05-28T10:50:56.122Z
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 954
File: tests/storage/storage_migration/conftest.py:264-269
Timestamp: 2025-05-28T10:50:56.122Z
Learning: In the openshift-virtualization-tests codebase, cleanup pytest fixtures like `deleted_old_dvs_of_stopped_vms`, `deleted_completed_virt_launcher_source_pod`, and `deleted_old_dvs_of_online_vms` do not require yield statements. These fixtures perform cleanup operations and work correctly without yielding values.

Applied to files:

  • tests/install_upgrade_operators/conftest.py
📚 Learning: 2025-09-29T20:33:51.007Z
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 1932
File: tests/virt/node/migration_and_maintenance/conftest.py:65-72
Timestamp: 2025-09-29T20:33:51.007Z
Learning: In tests/virt/node/migration_and_maintenance/conftest.py, the added_vm_cpu_limit fixture doesn't require ResourceEditor as a context manager because it's the final test to modify the vm_for_multifd_test VM before teardown, so restoration of CPU limits is unnecessary overhead as confirmed by maintainer dshchedr.

Applied to files:

  • tests/install_upgrade_operators/conftest.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: can-be-merged
  • GitHub Check: can-be-merged
  • GitHub Check: build-container
  • GitHub Check: tox
  • GitHub Check: can-be-merged
🔇 Additional comments (2)
tests/install_upgrade_operators/conftest.py (2)

207-222: LGTM! Fixture simplification is clean.

The removal of the is_jira_71823_open parameter and associated xfail logic correctly simplifies the fixture now that the bug is fixed. The function now has straightforward behavior: find and return the matching related object or raise an error.


24-24: Clean import removal, but verify broader cleanup.

The removal of HCO_BEARER_AUTH from line 24 is correct for this file since the constant is no longer used in the fixture logic. Ensure HCO_BEARER_AUTH, is_jira_71823_open, and any references to CNV-71823 are completely removed from other test files that may have depended on the xfail behavior.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants