Add admin_client to download_file_from_cluster #3353
Conversation
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
📝 WalkthroughWalkthroughTests now inject an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Contributor
|
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
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3353 +/- ##
=======================================
Coverage 98.38% 98.38%
=======================================
Files 25 25
Lines 2166 2167 +1
=======================================
+ Hits 2131 2132 +1
Misses 35 35
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:
|
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
previously approved these changes
Jan 7, 2026
![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
🤖 Fix all issues with AI agents
In @utilities/infra.py:
- Line 764: The function download_file_from_cluster is missing parameter and
return type annotations; update its signature to include types (e.g.,
get_console_spec_links_name: str, dest_dir: pathlib.Path or py.path.local (use
the project-preferred tmpdir type), admin_client: Any or the specific
AdminClient interface/class used in the repo) and add an explicit return type
(likely -> None). Import required types at top (from typing import Any; from
pathlib import Path or from py.path import local as LocalPath) and adjust the
docstring if needed to reflect the chosen dest_dir type.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests/conftest.pyutilities/infra.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: NEVER add linter suppressions like# noqa,# type: ignore,# pylint: disable— fix the code instead of suppressing linter errors
Google-format docstrings are REQUIRED for all public functions with non-obvious return values OR side effects
ALWAYS use absolute imports — NEVER use relative imports in Python files
ALWAYS import specific functions — usefrom module import func, NEVER useimport module
ALWAYS use named arguments for function calls with more than one argument
NEVER use single-letter variable names — ALWAYS use descriptive, meaningful names
No dead code — every function, variable, fixture MUST be used or removed. Code marked with# skip-unused-codeis excluded from dead code analysis
Prefer direct attribute access — usefoo.attrdirectly. Save to variables only when reusing the same attribute multiple times improves readability or extracting clarifies intent
INFO level logging REQUIRED for — test phase transitions, resource creation/deletion, configuration changes, API responses, intermediate state
ERROR level logging REQUIRED for — exceptions with full context: what failed, expected vs actual values, resource state
NEVER use DEBUG level logging — if a log is needed, use INFO
NEVER log secrets, tokens, passwords, or PII
Log format REQUIRED — structured key-value pairs for searchability using format likeLOGGER.info("VM created", extra={"name": vm_name, "namespace": ns})
ALWAYS re-raise exceptions with context — useraise NewError("message") from original_errorto preserve stack trace
Do not catch bareException— catch specific exception types only
NEVER silently swallow exceptions — at minimum, log the error before continuing
ALWAYS usewithfor resources — files, connections, locks MUST use context managers
ALWAYS usetimeout_samplerfromtimeout_samplerpackage for any operation that waits for a condition, NEVER usetime.sleep()in loops
Use implicit boolean checks —if items:NOTif len(items) > 0:or `if items...
Files:
utilities/infra.pytests/conftest.py
{libs,utilities}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Type hints are MANDATORY — mypy strict mode in
libs/, all new public functions underutilities/MUST be typed
Files:
utilities/infra.py
**/conftest.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/conftest.py: Fixtures MUST do ONE action only — single responsibility principle required
Fixture naming REQUIRED — ALWAYS use NOUNS (what they provide), NEVER verbs. Use descriptive names likevm_with_diskinstead ofcreate_vm_with_disk
Fixture parametrization format — userequest.paramwith dict structure for complex parameters
Fixture ordering REQUIRED — pytest native fixtures first, then session-scoped, then module/class/function scoped
Fixture scope rules — usescope="function"for setup requiring test isolation,scope="class"for setup shared across test class,scope="module"for expensive setup in a test module,scope="session"for setup persisting entire test run. NEVER use broader scope if fixture modifies state or creates per-test resources
Fixtures with cleanup MUST useyield— useyield resourcefollowed by cleanup code, NEVER return + finalizer
Files:
tests/conftest.py
🧠 Learnings (12)
📓 Common learnings
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1257-1258
Timestamp: 2025-08-06T13:57:34.740Z
Learning: In the openshift-virtualization-tests repository, all existing calls to the `get_cluster_platform` function in utilities/infra.py already pass the `admin_client` parameter, so the breaking change requiring this parameter does not actually break any existing code.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1249-1254
Timestamp: 2025-08-06T13:57:51.928Z
Learning: User rnetser verified that all calls to get_infrastructure() function use the admin_client parameter, confirming that signature changes requiring this parameter don't cause breaking changes in the openshift-virtualization-tests codebase.
Learnt from: SiboWang1997
Repo: RedHatQE/openshift-virtualization-tests PR: 3328
File: utilities/virt.py:1509-1512
Timestamp: 2026-01-06T09:35:16.926Z
Learning: In the openshift-virtualization-tests repository, when ServiceForVirtualMachineForTests.service_ip() needs to access Node information (for NODE_PORT service type), it must use cache_admin_client() rather than self.vm.client because Node is a cluster-scoped resource requiring admin permissions, even if the VM was created with an unprivileged client.
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 1716
File: tests/virt/node/workload_density/test_swap.py:71-85
Timestamp: 2025-08-09T01:46:48.039Z
Learning: In the openshift-virtualization-tests repository, user dshchedr prefers that all test setup fixtures be explicitly declared as parameters in the test method itself rather than chaining fixtures through dependencies. This makes all setup steps visible in one place at the test method level, improving test readability and making dependencies explicit, even if a fixture isn't directly used within another fixture's implementation.
📚 Learning: 2025-08-06T13:57:34.740Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1257-1258
Timestamp: 2025-08-06T13:57:34.740Z
Learning: In the openshift-virtualization-tests repository, all existing calls to the `get_cluster_platform` function in utilities/infra.py already pass the `admin_client` parameter, so the breaking change requiring this parameter does not actually break any existing code.
Applied to files:
utilities/infra.pytests/conftest.py
📚 Learning: 2025-08-06T13:57:51.928Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1249-1254
Timestamp: 2025-08-06T13:57:51.928Z
Learning: User rnetser verified that all calls to get_infrastructure() function use the admin_client parameter, confirming that signature changes requiring this parameter don't cause breaking changes in the openshift-virtualization-tests codebase.
Applied to files:
utilities/infra.pytests/conftest.py
📚 Learning: 2025-09-15T06:49:53.478Z
Learnt from: vsibirsk
Repo: RedHatQE/openshift-virtualization-tests PR: 2045
File: tests/virt/cluster/vm_lifecycle/conftest.py:46-47
Timestamp: 2025-09-15T06:49:53.478Z
Learning: In the openshift-virtualization-tests repo, large fixture refactoring efforts like the golden image data source migration are handled incrementally by directory/team ownership. The virt/cluster directory is handled separately from virt/node, tests/infra, tests/storage, etc., with each area managed by relevant teams in follow-up PRs.
Applied to files:
tests/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/conftest.py
📚 Learning: 2025-09-07T13:16:32.011Z
Learnt from: qwang1
Repo: RedHatQE/openshift-virtualization-tests PR: 1678
File: utilities/oadp.py:1-5
Timestamp: 2025-09-07T13:16:32.011Z
Learning: In the openshift-virtualization-tests project utilities/oadp.py, DynamicClient instances are passed as parameters rather than created internally, so kubernetes.config import is not needed for client creation fallback patterns.
Applied to files:
tests/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/conftest.py
📚 Learning: 2025-12-16T15:09:49.597Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3062
File: conftest.py:333-333
Timestamp: 2025-12-16T15:09:49.597Z
Learning: In the openshift-virtualization-tests repository, when conftest.py or utilities/bitwarden.py changes affect py_config["os_login_param"], smoke test impact must be determined by: (1) finding all smoke tests using `rg "pytest.mark.smoke"`, (2) checking each for VM creation patterns (VirtualMachineForTests, running_vm, VirtualMachineForTestsFromTemplate), (3) tracing whether running_vm is called with default check_ssh_connectivity=True, which accesses vm.login_params property that reads py_config["os_login_param"][vm.os_flavor]. The dependency chain is: smoke test → VM creation → running_vm → wait_for_ssh_connectivity → vm.login_params → os_login_param. Any smoke test creating VMs with SSH connectivity (the default) depends on os_login_param.
Applied to files:
tests/conftest.py
📚 Learning: 2025-12-16T20:11:03.645Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3062
File: conftest.py:333-333
Timestamp: 2025-12-16T20:11:03.645Z
Learning: In the openshift-virtualization-tests repository, when determining smoke test impact for changes affecting py_config["os_login_param"], follow this verification methodology: (1) Find all smoke tests: `rg "pytest.mark.smoke" --type=py -B2 | grep "def test_"`, (2) For each smoke test file, search for VM creation patterns: `rg "VirtualMachineForTests|running_vm|VirtualMachineForTestsFromTemplate|wait_for_ssh|check_ssh_connectivity"`, (3) Trace the dependency chain: smoke test → VirtualMachineForTests/running_vm() → wait_for_ssh_connectivity() (default enabled) → vm.login_params property → py_config["os_login_param"][vm.os_flavor], (4) Check utilities/virt.py for login_params usage: `rg "os_login_param|login_params" utilities/virt.py -C3`. Any smoke test creating VMs with default SSH connectivity checks (running_vm with check_ssh_connectivity=True) depends on os_login_param, even if the test doesn't directly reference it.
Applied to files:
tests/conftest.py
📚 Learning: 2026-01-06T09:35:16.926Z
Learnt from: SiboWang1997
Repo: RedHatQE/openshift-virtualization-tests PR: 3328
File: utilities/virt.py:1509-1512
Timestamp: 2026-01-06T09:35:16.926Z
Learning: In the openshift-virtualization-tests repository, when ServiceForVirtualMachineForTests.service_ip() needs to access Node information (for NODE_PORT service type), it must use cache_admin_client() rather than self.vm.client because Node is a cluster-scoped resource requiring admin permissions, even if the VM was created with an unprivileged client.
Applied to files:
tests/conftest.py
📚 Learning: 2025-12-15T12:33:06.686Z
Learnt from: azhivovk
Repo: RedHatQE/openshift-virtualization-tests PR: 3024
File: tests/network/connectivity/utils.py:17-17
Timestamp: 2025-12-15T12:33:06.686Z
Learning: In the test suite, ensure the ipv6_network_data fixture returns a factory function (Callable) and that all call sites invoke it to obtain the actual data dict, i.e., use ipv6_network_data() at call sites. This enables future extensibility for configuring secondary interfaces' IP addresses without changing call sites. Apply this pattern to all Python test files under tests.
Applied to files:
tests/conftest.py
📚 Learning: 2025-12-22T16:27:40.244Z
Learnt from: yossisegev
Repo: RedHatQE/openshift-virtualization-tests PR: 3196
File: tests/network/upgrade/test_upgrade_network.py:4-4
Timestamp: 2025-12-22T16:27:40.244Z
Learning: For PRs that remove tests, rely on pytest --collect-only to verify the test discovery results (which tests are selected/deselected) and ensure the removal is clean and the test module remains functional. Full test execution is not required for test deletion PRs. This guideline applies to test files anywhere under the tests/ directory (e.g., tests/network/upgrade/test_upgrade_network.py) and should be used for similar test-deletion scenarios across the repository.
Applied to files:
tests/conftest.py
🧬 Code graph analysis (2)
utilities/infra.py (1)
tests/conftest.py (1)
admin_client(295-299)
tests/conftest.py (1)
utilities/infra.py (1)
download_file_from_cluster(764-782)
🪛 Ruff (0.14.10)
tests/conftest.py
1934-1934: Unused function argument: os_path_environment
(ARG001)
1939-1939: Logging statement uses f-string
(G004)
1947-1947: Unused function argument: os_path_environment
(ARG001)
1950-1950: Logging statement uses f-string
(G004)
⏰ 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). (8)
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: build-container
- GitHub Check: tox
🔇 Additional comments (3)
tests/conftest.py (2)
1934-1943: LGTM! Clean dependency injection.The
admin_clientparameter is correctly added to the fixture signature and properly threaded through todownload_file_from_cluster. The session scope is appropriate sinceadmin_clientis also session-scoped (line 295).The static analysis warning about
os_path_environmentbeing unused is a false positive - the parameter is used for pytest fixture dependency ordering, ensuring theos_path_environmentfixture runs before this one.
1947-1954: LGTM! Consistent with thevirtctl_binaryfixture.The
admin_clientparameter is correctly added and used identically to thevirtctl_binaryfixture above. The changes maintain consistency across both binary download fixtures.utilities/infra.py (1)
764-782: All callers ofdownload_file_from_clusterhave been updated correctly. The codebase contains exactly two callers (both intests/conftest.pyat lines 1941 and 1952), and both pass theadmin_clientparameter using named arguments. The function signature change does not introduce breaking changes.
RoniKishner
force-pushed
the
infra-get-client
branch
from
4b31e76 to
5dcbb67
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
Contributor
Author
|
/verified |
openshift-virtualization-qe-bot
added
tox:verify-bugs-are-open:running
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @tests/conftest.py:
- Around line 1947-1954: In the oc_binary fixture remove the unused parameter
os_path_environment from the function signature and any references so the
dependency is provided by bin_directory_to_os_path instead; also replace the
f-string LOGGER.warning call in oc_binary (currently logging f"Using previously
installed: {installed_oc}") with structured logging using LOGGER.warning and an
extra dict (e.g., extra={"installed_oc": installed_oc}) or analogous structured
fields so the message and data are logged separately.
- Around line 1934-1943: The fixture virtctl_binary currently declares an unused
parameter os_path_environment and logs with an f-string; remove
os_path_environment from the virtctl_binary signature and update any
references/tests if they directly call it (the existing fixture ordering via
bin_directory_to_os_path already enforces dependency so this is safe), add
appropriate type hints to virtctl_binary parameters and its return type, and
replace the f-string LOGGER.warning call with structured logging (e.g.,
LOGGER.warning("Using previously installed virtctl", extra={"path":
installed_virtctl})).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests/conftest.pyutilities/infra.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: NEVER add linter suppressions like# noqa,# type: ignore,# pylint: disable— fix the code instead of suppressing linter errors
Google-format docstrings are REQUIRED for all public functions with non-obvious return values OR side effects
ALWAYS use absolute imports — NEVER use relative imports in Python files
ALWAYS import specific functions — usefrom module import func, NEVER useimport module
ALWAYS use named arguments for function calls with more than one argument
NEVER use single-letter variable names — ALWAYS use descriptive, meaningful names
No dead code — every function, variable, fixture MUST be used or removed. Code marked with# skip-unused-codeis excluded from dead code analysis
Prefer direct attribute access — usefoo.attrdirectly. Save to variables only when reusing the same attribute multiple times improves readability or extracting clarifies intent
INFO level logging REQUIRED for — test phase transitions, resource creation/deletion, configuration changes, API responses, intermediate state
ERROR level logging REQUIRED for — exceptions with full context: what failed, expected vs actual values, resource state
NEVER use DEBUG level logging — if a log is needed, use INFO
NEVER log secrets, tokens, passwords, or PII
Log format REQUIRED — structured key-value pairs for searchability using format likeLOGGER.info("VM created", extra={"name": vm_name, "namespace": ns})
ALWAYS re-raise exceptions with context — useraise NewError("message") from original_errorto preserve stack trace
Do not catch bareException— catch specific exception types only
NEVER silently swallow exceptions — at minimum, log the error before continuing
ALWAYS usewithfor resources — files, connections, locks MUST use context managers
ALWAYS usetimeout_samplerfromtimeout_samplerpackage for any operation that waits for a condition, NEVER usetime.sleep()in loops
Use implicit boolean checks —if items:NOTif len(items) > 0:or `if items...
Files:
tests/conftest.pyutilities/infra.py
**/conftest.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/conftest.py: Fixtures MUST do ONE action only — single responsibility principle required
Fixture naming REQUIRED — ALWAYS use NOUNS (what they provide), NEVER verbs. Use descriptive names likevm_with_diskinstead ofcreate_vm_with_disk
Fixture parametrization format — userequest.paramwith dict structure for complex parameters
Fixture ordering REQUIRED — pytest native fixtures first, then session-scoped, then module/class/function scoped
Fixture scope rules — usescope="function"for setup requiring test isolation,scope="class"for setup shared across test class,scope="module"for expensive setup in a test module,scope="session"for setup persisting entire test run. NEVER use broader scope if fixture modifies state or creates per-test resources
Fixtures with cleanup MUST useyield— useyield resourcefollowed by cleanup code, NEVER return + finalizer
Files:
tests/conftest.py
{libs,utilities}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Type hints are MANDATORY — mypy strict mode in
libs/, all new public functions underutilities/MUST be typed
Files:
utilities/infra.py
🧠 Learnings (18)
📓 Common learnings
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1257-1258
Timestamp: 2025-08-06T13:57:34.740Z
Learning: In the openshift-virtualization-tests repository, all existing calls to the `get_cluster_platform` function in utilities/infra.py already pass the `admin_client` parameter, so the breaking change requiring this parameter does not actually break any existing code.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1249-1254
Timestamp: 2025-08-06T13:57:51.928Z
Learning: User rnetser verified that all calls to get_infrastructure() function use the admin_client parameter, confirming that signature changes requiring this parameter don't cause breaking changes in the openshift-virtualization-tests codebase.
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-12-22T15:56:00.157Z
Learning: In the openshift-virtualization-tests repository, when responding to test execution plan requests from openshift-virtualization-qe-bot-3, do NOT use "REQUEST_CHANGES" review type if the PR author has already marked the PR as verified (e.g., with `/verified` command). Test execution plans are informational guides, not blocking requirements. Use COMMENT event for informational test plans, or only REQUEST_CHANGES if there are actual code issues that need to be addressed before merging.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-12-16T14:00:59.076Z
Learning: In the openshift-virtualization-tests repository, when responding to test execution plan requests from openshift-virtualization-qe-bot-3, CodeRabbit must post ONLY an inline review comment on the Files Changed tab and then stop immediately without generating any follow-up comments in the PR discussion thread. No acknowledgment messages, no confirmation of posting, no explanation - silence after posting the inline review equals success. Additional comments create empty/meaningless reviews that clutter the PR.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3228
File: .coderabbit.yaml:30-41
Timestamp: 2026-01-05T10:33:50.728Z
Learning: In the openshift-virtualization-tests repository, user rnetser prefers minimal pre-merge checks in CodeRabbit configuration: only docstrings enforcement (80% threshold) is needed, not title or description checks.
📚 Learning: 2025-08-06T13:57:34.740Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1257-1258
Timestamp: 2025-08-06T13:57:34.740Z
Learning: In the openshift-virtualization-tests repository, all existing calls to the `get_cluster_platform` function in utilities/infra.py already pass the `admin_client` parameter, so the breaking change requiring this parameter does not actually break any existing code.
Applied to files:
tests/conftest.pyutilities/infra.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/conftest.py
📚 Learning: 2025-09-15T06:49:53.478Z
Learnt from: vsibirsk
Repo: RedHatQE/openshift-virtualization-tests PR: 2045
File: tests/virt/cluster/vm_lifecycle/conftest.py:46-47
Timestamp: 2025-09-15T06:49:53.478Z
Learning: In the openshift-virtualization-tests repo, large fixture refactoring efforts like the golden image data source migration are handled incrementally by directory/team ownership. The virt/cluster directory is handled separately from virt/node, tests/infra, tests/storage, etc., with each area managed by relevant teams in follow-up PRs.
Applied to files:
tests/conftest.py
📚 Learning: 2025-08-06T13:57:51.928Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1249-1254
Timestamp: 2025-08-06T13:57:51.928Z
Learning: User rnetser verified that all calls to get_infrastructure() function use the admin_client parameter, confirming that signature changes requiring this parameter don't cause breaking changes in the openshift-virtualization-tests codebase.
Applied to files:
tests/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/conftest.py
📚 Learning: 2025-12-16T15:09:49.597Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3062
File: conftest.py:333-333
Timestamp: 2025-12-16T15:09:49.597Z
Learning: In the openshift-virtualization-tests repository, when conftest.py or utilities/bitwarden.py changes affect py_config["os_login_param"], smoke test impact must be determined by: (1) finding all smoke tests using `rg "pytest.mark.smoke"`, (2) checking each for VM creation patterns (VirtualMachineForTests, running_vm, VirtualMachineForTestsFromTemplate), (3) tracing whether running_vm is called with default check_ssh_connectivity=True, which accesses vm.login_params property that reads py_config["os_login_param"][vm.os_flavor]. The dependency chain is: smoke test → VM creation → running_vm → wait_for_ssh_connectivity → vm.login_params → os_login_param. Any smoke test creating VMs with SSH connectivity (the default) depends on os_login_param.
Applied to files:
tests/conftest.py
📚 Learning: 2025-12-16T20:11:03.645Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3062
File: conftest.py:333-333
Timestamp: 2025-12-16T20:11:03.645Z
Learning: In the openshift-virtualization-tests repository, when determining smoke test impact for changes affecting py_config["os_login_param"], follow this verification methodology: (1) Find all smoke tests: `rg "pytest.mark.smoke" --type=py -B2 | grep "def test_"`, (2) For each smoke test file, search for VM creation patterns: `rg "VirtualMachineForTests|running_vm|VirtualMachineForTestsFromTemplate|wait_for_ssh|check_ssh_connectivity"`, (3) Trace the dependency chain: smoke test → VirtualMachineForTests/running_vm() → wait_for_ssh_connectivity() (default enabled) → vm.login_params property → py_config["os_login_param"][vm.os_flavor], (4) Check utilities/virt.py for login_params usage: `rg "os_login_param|login_params" utilities/virt.py -C3`. Any smoke test creating VMs with default SSH connectivity checks (running_vm with check_ssh_connectivity=True) depends on os_login_param, even if the test doesn't directly reference it.
Applied to files:
tests/conftest.py
📚 Learning: 2025-08-04T15:27:14.175Z
Learnt from: OhadRevah
Repo: RedHatQE/openshift-virtualization-tests PR: 1166
File: tests/observability/metrics/conftest.py:1065-1082
Timestamp: 2025-08-04T15:27:14.175Z
Learning: In tests/observability/metrics/conftest.py, the `non_existent_node_windows_vm` fixture is used for tier3 Windows VM testing and must use Windows VMs rather than Linux VMs, even though it adds overhead, because it's specifically testing Windows VM status transition metrics as part of dedicated Windows VM test coverage.
Applied to files:
tests/conftest.py
📚 Learning: 2025-12-16T10:28:54.212Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-12-16T10:28:54.212Z
Learning: In the openshift-virtualization-tests repository, when determining smoke test impact for test execution plans, only set "Run smoke tests: True" if there is a verified, traceable dependency path from smoke tests to the changed code. Session-scoped fixtures or infrastructure-sounding changes do NOT automatically imply smoke test impact - the dependency chain must be explicitly verified using shell scripts before recommending smoke test execution.
Applied to files:
tests/conftest.py
📚 Learning: 2026-01-04T19:37:57.919Z
Learnt from: SamAlber
Repo: RedHatQE/openshift-virtualization-tests PR: 3027
File: tests/install_upgrade_operators/strict_reconciliation/test_hco_default_cpu_model.py:123-148
Timestamp: 2026-01-04T19:37:57.919Z
Learning: In tests/install_upgrade_operators/strict_reconciliation/test_hco_default_cpu_model.py, the test pattern intentionally uses class-scoped fixtures to modify HCO resources (e.g., hco_with_default_cpu_model_set patching hyperconverged_resource_scope_class) while function-scoped fixtures (e.g., hyperconverged_resource_scope_function) fetch fresh cluster state in tests. This verifies that HCO reconciliation has propagated correctly by comparing the fresh state with the expected value set by the class-scoped modification.
Applied to files:
tests/conftest.py
📚 Learning: 2025-09-29T19:05:24.987Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-09-29T19:05:24.987Z
Learning: The test execution plan for PR #1904 focuses on cluster-type conditional logic where nmstate functionality is bypassed on cloud clusters (Azure/AWS) but fully functional on bare-metal/PSI clusters, requiring different test strategies for each environment type.
Applied to files:
tests/conftest.py
📚 Learning: 2025-09-29T19:05:24.987Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-09-29T19:05:24.987Z
Learning: For PR #1904 test execution, the critical validation point is test_connectivity_over_migration_between_localnet_vms which should fail gracefully on cloud clusters but pass on bare-metal/PSI clusters, representing the core nmstate conditional logic functionality.
Applied to files:
tests/conftest.py
📚 Learning: 2025-12-29T10:07:12.804Z
Learnt from: hmeir
Repo: RedHatQE/openshift-virtualization-tests PR: 3081
File: tests/install_upgrade_operators/product_install/test_install_openshift_virtualization.py:189-191
Timestamp: 2025-12-29T10:07:12.804Z
Learning: In test_ocs_virt_default_storage_class in tests/install_upgrade_operators/product_install/test_install_openshift_virtualization.py, consecutive_checks_count=3 is sufficient for verify_boot_sources_reimported because datasources update quickly in the installation lane scenario after storage class changes.
Applied to files:
tests/conftest.py
📚 Learning: 2026-01-06T09:35:16.926Z
Learnt from: SiboWang1997
Repo: RedHatQE/openshift-virtualization-tests PR: 3328
File: utilities/virt.py:1509-1512
Timestamp: 2026-01-06T09:35:16.926Z
Learning: In the openshift-virtualization-tests repository, when ServiceForVirtualMachineForTests.service_ip() needs to access Node information (for NODE_PORT service type), it must use cache_admin_client() rather than self.vm.client because Node is a cluster-scoped resource requiring admin permissions, even if the VM was created with an unprivileged client.
Applied to files:
tests/conftest.py
📚 Learning: 2025-12-15T12:33:06.686Z
Learnt from: azhivovk
Repo: RedHatQE/openshift-virtualization-tests PR: 3024
File: tests/network/connectivity/utils.py:17-17
Timestamp: 2025-12-15T12:33:06.686Z
Learning: In the test suite, ensure the ipv6_network_data fixture returns a factory function (Callable) and that all call sites invoke it to obtain the actual data dict, i.e., use ipv6_network_data() at call sites. This enables future extensibility for configuring secondary interfaces' IP addresses without changing call sites. Apply this pattern to all Python test files under tests.
Applied to files:
tests/conftest.py
📚 Learning: 2025-12-22T16:27:40.244Z
Learnt from: yossisegev
Repo: RedHatQE/openshift-virtualization-tests PR: 3196
File: tests/network/upgrade/test_upgrade_network.py:4-4
Timestamp: 2025-12-22T16:27:40.244Z
Learning: For PRs that remove tests, rely on pytest --collect-only to verify the test discovery results (which tests are selected/deselected) and ensure the removal is clean and the test module remains functional. Full test execution is not required for test deletion PRs. This guideline applies to test files anywhere under the tests/ directory (e.g., tests/network/upgrade/test_upgrade_network.py) and should be used for similar test-deletion scenarios across the repository.
Applied to files:
tests/conftest.py
📚 Learning: 2026-01-06T20:32:10.833Z
Learnt from: CR
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T20:32:10.833Z
Learning: Applies to {libs,utilities}/**/*.py : Type hints are MANDATORY — mypy strict mode in `libs/`, all new public functions under `utilities/` MUST be typed
Applied to files:
utilities/infra.py
🧬 Code graph analysis (2)
tests/conftest.py (1)
utilities/infra.py (1)
download_file_from_cluster(764-784)
utilities/infra.py (1)
tests/conftest.py (1)
admin_client(295-299)
🪛 Ruff (0.14.10)
tests/conftest.py
1934-1934: Unused function argument: os_path_environment
(ARG001)
1939-1939: Logging statement uses f-string
(G004)
1947-1947: Unused function argument: os_path_environment
(ARG001)
1950-1950: Logging statement uses f-string
(G004)
⏰ 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). (8)
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: build-container
- GitHub Check: tox
🔇 Additional comments (1)
utilities/infra.py (1)
764-784: LGTM! Proper dependency injection implemented.The function signature now correctly accepts
admin_clientas a parameter and passes it toget_console_spec_linksinstead of callingget_client()internally. This aligns with the PR objective to eliminateget_client()usage and improves testability through dependency injection.The type hints are appropriate and the function logic remains unchanged beyond the client parameter threading.
Signed-off-by: Roni Kishner <rkishner@redhat.com>
RoniKishner
force-pushed
the
infra-get-client
branch
from
a573fb3 to
7e15c44
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
approved these changes
Jan 7, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/conftest.py:
- Line 1939: The log call using an f-string should be converted to structured
key-value logging; replace LOGGER.warning(f"Using previously installed:
{installed_virtctl}") in the virtctl_binary fixture (and the similar call in
oc_binary) with a structured call that supplies a message and the installed path
in extra (e.g., LOGGER.warning("Using previously installed virtctl",
extra={"path": installed_virtctl})), ensuring both fixtures use the LOGGER
object and follow the project's structured logging pattern.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests/conftest.pyutilities/infra.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: NEVER add linter suppressions like# noqa,# type: ignore,# pylint: disable— fix the code instead of suppressing linter errors
Google-format docstrings are REQUIRED for all public functions with non-obvious return values OR side effects
ALWAYS use absolute imports — NEVER use relative imports in Python files
ALWAYS import specific functions — usefrom module import func, NEVER useimport module
ALWAYS use named arguments for function calls with more than one argument
NEVER use single-letter variable names — ALWAYS use descriptive, meaningful names
No dead code — every function, variable, fixture MUST be used or removed. Code marked with# skip-unused-codeis excluded from dead code analysis
Prefer direct attribute access — usefoo.attrdirectly. Save to variables only when reusing the same attribute multiple times improves readability or extracting clarifies intent
INFO level logging REQUIRED for — test phase transitions, resource creation/deletion, configuration changes, API responses, intermediate state
ERROR level logging REQUIRED for — exceptions with full context: what failed, expected vs actual values, resource state
NEVER use DEBUG level logging — if a log is needed, use INFO
NEVER log secrets, tokens, passwords, or PII
Log format REQUIRED — structured key-value pairs for searchability using format likeLOGGER.info("VM created", extra={"name": vm_name, "namespace": ns})
ALWAYS re-raise exceptions with context — useraise NewError("message") from original_errorto preserve stack trace
Do not catch bareException— catch specific exception types only
NEVER silently swallow exceptions — at minimum, log the error before continuing
ALWAYS usewithfor resources — files, connections, locks MUST use context managers
ALWAYS usetimeout_samplerfromtimeout_samplerpackage for any operation that waits for a condition, NEVER usetime.sleep()in loops
Use implicit boolean checks —if items:NOTif len(items) > 0:or `if items...
Files:
utilities/infra.pytests/conftest.py
{libs,utilities}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Type hints are MANDATORY — mypy strict mode in
libs/, all new public functions underutilities/MUST be typed
Files:
utilities/infra.py
**/conftest.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/conftest.py: Fixtures MUST do ONE action only — single responsibility principle required
Fixture naming REQUIRED — ALWAYS use NOUNS (what they provide), NEVER verbs. Use descriptive names likevm_with_diskinstead ofcreate_vm_with_disk
Fixture parametrization format — userequest.paramwith dict structure for complex parameters
Fixture ordering REQUIRED — pytest native fixtures first, then session-scoped, then module/class/function scoped
Fixture scope rules — usescope="function"for setup requiring test isolation,scope="class"for setup shared across test class,scope="module"for expensive setup in a test module,scope="session"for setup persisting entire test run. NEVER use broader scope if fixture modifies state or creates per-test resources
Fixtures with cleanup MUST useyield— useyield resourcefollowed by cleanup code, NEVER return + finalizer
Files:
tests/conftest.py
🧠 Learnings (26)
📓 Common learnings
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1257-1258
Timestamp: 2025-08-06T13:57:34.740Z
Learning: In the openshift-virtualization-tests repository, all existing calls to the `get_cluster_platform` function in utilities/infra.py already pass the `admin_client` parameter, so the breaking change requiring this parameter does not actually break any existing code.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1249-1254
Timestamp: 2025-08-06T13:57:51.928Z
Learning: User rnetser verified that all calls to get_infrastructure() function use the admin_client parameter, confirming that signature changes requiring this parameter don't cause breaking changes in the openshift-virtualization-tests codebase.
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-12-22T15:56:00.157Z
Learning: In the openshift-virtualization-tests repository, when responding to test execution plan requests from openshift-virtualization-qe-bot-3, do NOT use "REQUEST_CHANGES" review type if the PR author has already marked the PR as verified (e.g., with `/verified` command). Test execution plans are informational guides, not blocking requirements. Use COMMENT event for informational test plans, or only REQUEST_CHANGES if there are actual code issues that need to be addressed before merging.
Learnt from: qwang1
Repo: RedHatQE/openshift-virtualization-tests PR: 1678
File: utilities/oadp.py:1-5
Timestamp: 2025-09-07T13:16:32.011Z
Learning: In the openshift-virtualization-tests project utilities/oadp.py, DynamicClient instances are passed as parameters rather than created internally, so kubernetes.config import is not needed for client creation fallback patterns.
📚 Learning: 2025-08-06T13:57:34.740Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1257-1258
Timestamp: 2025-08-06T13:57:34.740Z
Learning: In the openshift-virtualization-tests repository, all existing calls to the `get_cluster_platform` function in utilities/infra.py already pass the `admin_client` parameter, so the breaking change requiring this parameter does not actually break any existing code.
Applied to files:
utilities/infra.pytests/conftest.py
📚 Learning: 2025-08-06T13:57:51.928Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1249-1254
Timestamp: 2025-08-06T13:57:51.928Z
Learning: User rnetser verified that all calls to get_infrastructure() function use the admin_client parameter, confirming that signature changes requiring this parameter don't cause breaking changes in the openshift-virtualization-tests codebase.
Applied to files:
utilities/infra.pytests/conftest.py
📚 Learning: 2026-01-06T20:32:10.833Z
Learnt from: CR
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T20:32:10.833Z
Learning: Applies to {libs,utilities}/**/*.py : Type hints are MANDATORY — mypy strict mode in `libs/`, all new public functions under `utilities/` MUST be typed
Applied to files:
utilities/infra.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/conftest.py
📚 Learning: 2025-09-15T06:49:53.478Z
Learnt from: vsibirsk
Repo: RedHatQE/openshift-virtualization-tests PR: 2045
File: tests/virt/cluster/vm_lifecycle/conftest.py:46-47
Timestamp: 2025-09-15T06:49:53.478Z
Learning: In the openshift-virtualization-tests repo, large fixture refactoring efforts like the golden image data source migration are handled incrementally by directory/team ownership. The virt/cluster directory is handled separately from virt/node, tests/infra, tests/storage, etc., with each area managed by relevant teams in follow-up PRs.
Applied to files:
tests/conftest.py
📚 Learning: 2025-12-16T15:09:49.597Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3062
File: conftest.py:333-333
Timestamp: 2025-12-16T15:09:49.597Z
Learning: In the openshift-virtualization-tests repository, when conftest.py or utilities/bitwarden.py changes affect py_config["os_login_param"], smoke test impact must be determined by: (1) finding all smoke tests using `rg "pytest.mark.smoke"`, (2) checking each for VM creation patterns (VirtualMachineForTests, running_vm, VirtualMachineForTestsFromTemplate), (3) tracing whether running_vm is called with default check_ssh_connectivity=True, which accesses vm.login_params property that reads py_config["os_login_param"][vm.os_flavor]. The dependency chain is: smoke test → VM creation → running_vm → wait_for_ssh_connectivity → vm.login_params → os_login_param. Any smoke test creating VMs with SSH connectivity (the default) depends on os_login_param.
Applied to files:
tests/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/conftest.py
📚 Learning: 2025-12-16T20:11:03.645Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3062
File: conftest.py:333-333
Timestamp: 2025-12-16T20:11:03.645Z
Learning: In the openshift-virtualization-tests repository, when determining smoke test impact for changes affecting py_config["os_login_param"], follow this verification methodology: (1) Find all smoke tests: `rg "pytest.mark.smoke" --type=py -B2 | grep "def test_"`, (2) For each smoke test file, search for VM creation patterns: `rg "VirtualMachineForTests|running_vm|VirtualMachineForTestsFromTemplate|wait_for_ssh|check_ssh_connectivity"`, (3) Trace the dependency chain: smoke test → VirtualMachineForTests/running_vm() → wait_for_ssh_connectivity() (default enabled) → vm.login_params property → py_config["os_login_param"][vm.os_flavor], (4) Check utilities/virt.py for login_params usage: `rg "os_login_param|login_params" utilities/virt.py -C3`. Any smoke test creating VMs with default SSH connectivity checks (running_vm with check_ssh_connectivity=True) depends on os_login_param, even if the test doesn't directly reference it.
Applied to files:
tests/conftest.py
📚 Learning: 2025-12-16T10:28:54.212Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-12-16T10:28:54.212Z
Learning: In the openshift-virtualization-tests repository, when determining smoke test impact for test execution plans, only set "Run smoke tests: True" if there is a verified, traceable dependency path from smoke tests to the changed code. Session-scoped fixtures or infrastructure-sounding changes do NOT automatically imply smoke test impact - the dependency chain must be explicitly verified using shell scripts before recommending smoke test execution.
Applied to files:
tests/conftest.py
📚 Learning: 2025-08-04T15:27:14.175Z
Learnt from: OhadRevah
Repo: RedHatQE/openshift-virtualization-tests PR: 1166
File: tests/observability/metrics/conftest.py:1065-1082
Timestamp: 2025-08-04T15:27:14.175Z
Learning: In tests/observability/metrics/conftest.py, the `non_existent_node_windows_vm` fixture is used for tier3 Windows VM testing and must use Windows VMs rather than Linux VMs, even though it adds overhead, because it's specifically testing Windows VM status transition metrics as part of dedicated Windows VM test coverage.
Applied to files:
tests/conftest.py
📚 Learning: 2025-08-09T01:46:48.039Z
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 1716
File: tests/virt/node/workload_density/test_swap.py:71-85
Timestamp: 2025-08-09T01:46:48.039Z
Learning: In the openshift-virtualization-tests repository, user dshchedr prefers that all test setup fixtures be explicitly declared as parameters in the test method itself rather than chaining fixtures through dependencies. This makes all setup steps visible in one place at the test method level, improving test readability and making dependencies explicit, even if a fixture isn't directly used within another fixture's implementation.
Applied to files:
tests/conftest.py
📚 Learning: 2026-01-04T19:37:57.919Z
Learnt from: SamAlber
Repo: RedHatQE/openshift-virtualization-tests PR: 3027
File: tests/install_upgrade_operators/strict_reconciliation/test_hco_default_cpu_model.py:123-148
Timestamp: 2026-01-04T19:37:57.919Z
Learning: In tests/install_upgrade_operators/strict_reconciliation/test_hco_default_cpu_model.py, the test pattern intentionally uses class-scoped fixtures to modify HCO resources (e.g., hco_with_default_cpu_model_set patching hyperconverged_resource_scope_class) while function-scoped fixtures (e.g., hyperconverged_resource_scope_function) fetch fresh cluster state in tests. This verifies that HCO reconciliation has propagated correctly by comparing the fresh state with the expected value set by the class-scoped modification.
Applied to files:
tests/conftest.py
📚 Learning: 2025-09-29T19:05:24.987Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-09-29T19:05:24.987Z
Learning: The test execution plan for PR #1904 focuses on cluster-type conditional logic where nmstate functionality is bypassed on cloud clusters (Azure/AWS) but fully functional on bare-metal/PSI clusters, requiring different test strategies for each environment type.
Applied to files:
tests/conftest.py
📚 Learning: 2026-01-07T09:52:12.342Z
Learnt from: yossisegev
Repo: RedHatQE/openshift-virtualization-tests PR: 3358
File: tests/network/sriov/test_sriov.py:21-21
Timestamp: 2026-01-07T09:52:12.342Z
Learning: When a PR only removes or modifies pytest markers in tests (e.g., removing pytest.mark.post_upgrade) and the test logic remains unchanged, prefer verifying with pytest --collect-only instead of running the full test suite. This validates that marker usage and test selection behavior are preserved. If the test logic changes, or markers affect behavior beyond collection, run the full test suite to confirm.
Applied to files:
tests/conftest.py
📚 Learning: 2025-12-29T10:07:12.804Z
Learnt from: hmeir
Repo: RedHatQE/openshift-virtualization-tests PR: 3081
File: tests/install_upgrade_operators/product_install/test_install_openshift_virtualization.py:189-191
Timestamp: 2025-12-29T10:07:12.804Z
Learning: In test_ocs_virt_default_storage_class in tests/install_upgrade_operators/product_install/test_install_openshift_virtualization.py, consecutive_checks_count=3 is sufficient for verify_boot_sources_reimported because datasources update quickly in the installation lane scenario after storage class changes.
Applied to files:
tests/conftest.py
📚 Learning: 2025-09-29T19:05:24.987Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-09-29T19:05:24.987Z
Learning: For PR #1904 test execution, the critical validation point is test_connectivity_over_migration_between_localnet_vms which should fail gracefully on cloud clusters but pass on bare-metal/PSI clusters, representing the core nmstate conditional logic functionality.
Applied to files:
tests/conftest.py
📚 Learning: 2025-06-18T09:19:05.769Z
Learnt from: OhadRevah
Repo: RedHatQE/openshift-virtualization-tests PR: 1166
File: tests/observability/metrics/test_vms_metrics.py:129-137
Timestamp: 2025-06-18T09:19:05.769Z
Learning: For Windows VM testing in tests/observability/metrics/test_vms_metrics.py, it's acceptable to have more fixture parameters than typical pylint recommendations when reusing expensive Windows VM fixtures for performance. Windows VMs take a long time to deploy, so reusing fixtures like windows_vm_for_test and adding labels via windows_vm_with_low_bandwidth_migration_policy is preferred over creating separate fixtures that would require additional VM deployments.
Applied to files:
tests/conftest.py
📚 Learning: 2025-11-26T16:03:07.813Z
Learnt from: azhivovk
Repo: RedHatQE/openshift-virtualization-tests PR: 2119
File: tests/network/localnet/conftest.py:372-387
Timestamp: 2025-11-26T16:03:07.813Z
Learning: In the openshift-virtualization-tests repository, pytest fixtures that declare other fixtures as parameters purely for dependency ordering (without referencing them in the function body) should not be modified to silence Ruff ARG001 warnings. This is an idiomatic pytest pattern for ensuring setup order, and the team prefers to leave such fixtures unchanged rather than adding defensive comments or code to suppress linter warnings.
Applied to files:
tests/conftest.py
📚 Learning: 2026-01-06T20:32:10.833Z
Learnt from: CR
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T20:32:10.833Z
Learning: Applies to **/conftest.py : Fixture naming REQUIRED — ALWAYS use NOUNS (what they provide), NEVER verbs. Use descriptive names like `vm_with_disk` instead of `create_vm_with_disk`
Applied to files:
tests/conftest.py
📚 Learning: 2025-06-19T09:56:06.185Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1244
File: tests/virt/cluster/common_templates/windows/test_windows_custom_options.py:181-181
Timestamp: 2025-06-19T09:56:06.185Z
Learning: In tests/virt/cluster/common_templates/windows/test_windows_custom_options.py, the change from os.path.join to f-string path construction is intentional to handle cases where Images.Windows.WIN2k19_HA_IMG may be None during test collection on architectures where Windows images aren't available. The f-string approach allows pytest collection to succeed gracefully, while actual test execution is controlled by markers and environmental conditions.
Applied to files:
tests/conftest.py
📚 Learning: 2025-09-14T05:39:47.969Z
Learnt from: RoniKishner
Repo: RedHatQE/openshift-virtualization-tests PR: 1946
File: tests/infrastructure/instance_types/supported_os/conftest.py:117-131
Timestamp: 2025-09-14T05:39:47.969Z
Learning: In utilities/virt.py, Windows OS detection logic uses substring containment checks (e.g., `OS_FLAVOR_WINDOWS in self.os_flavor`) instead of exact equality, allowing os_flavor values like "win-container-disk" to correctly trigger Windows-specific configurations for memory, CPU, SSH authentication, and cloud-init settings.
Applied to files:
tests/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/conftest.py
📚 Learning: 2025-06-17T07:45:37.776Z
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 1160
File: tests/storage/storage_migration/test_mtc_storage_class_migration.py:165-176
Timestamp: 2025-06-17T07:45:37.776Z
Learning: In the openshift-virtualization-tests repository, user jpeimer prefers explicit fixture parameters over composite fixtures in test methods, even when there are many parameters, as they find this approach more readable and maintainable for understanding test dependencies.
Applied to files:
tests/conftest.py
📚 Learning: 2025-12-15T12:33:06.686Z
Learnt from: azhivovk
Repo: RedHatQE/openshift-virtualization-tests PR: 3024
File: tests/network/connectivity/utils.py:17-17
Timestamp: 2025-12-15T12:33:06.686Z
Learning: In the test suite, ensure the ipv6_network_data fixture returns a factory function (Callable) and that all call sites invoke it to obtain the actual data dict, i.e., use ipv6_network_data() at call sites. This enables future extensibility for configuring secondary interfaces' IP addresses without changing call sites. Apply this pattern to all Python test files under tests.
Applied to files:
tests/conftest.py
📚 Learning: 2025-12-22T16:27:40.244Z
Learnt from: yossisegev
Repo: RedHatQE/openshift-virtualization-tests PR: 3196
File: tests/network/upgrade/test_upgrade_network.py:4-4
Timestamp: 2025-12-22T16:27:40.244Z
Learning: For PRs that remove tests, rely on pytest --collect-only to verify the test discovery results (which tests are selected/deselected) and ensure the removal is clean and the test module remains functional. Full test execution is not required for test deletion PRs. This guideline applies to test files anywhere under the tests/ directory (e.g., tests/network/upgrade/test_upgrade_network.py) and should be used for similar test-deletion scenarios across the repository.
Applied to files:
tests/conftest.py
🧬 Code graph analysis (2)
utilities/infra.py (1)
tests/conftest.py (1)
admin_client(295-299)
tests/conftest.py (1)
utilities/infra.py (1)
download_file_from_cluster(764-784)
🪛 Ruff (0.14.10)
tests/conftest.py
1939-1939: Logging statement uses f-string
(G004)
1950-1950: Logging statement uses f-string
(G004)
⏰ 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). (6)
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: build-container
- GitHub Check: tox
- GitHub Check: can-be-merged
🔇 Additional comments (3)
utilities/infra.py (1)
764-770: LGTM! Clean dependency injection pattern.The function signature is properly typed and the
admin_clientparameter is correctly passed toget_console_spec_links. This change successfully eliminates the implicit dependency onget_client()by making the client dependency explicit.tests/conftest.py (2)
1934-1934: LGTM! Fixture correctly updated with admin_client parameter.The
virtctl_binaryfixture now accepts and passesadmin_clienttodownload_file_from_cluster, successfully implementing the dependency injection pattern. The removal of the unusedos_path_environmentparameter is also correct—dependency ordering is properly maintained via thebin_directory_to_os_pathfixture at line 1958.Also applies to: 1941-1943
1947-1947: LGTM! Consistent fixture update pattern.The
oc_binaryfixture mirrors thevirtctl_binarychanges correctly—admin_clientparameter added and passed todownload_file_from_cluster, unusedos_path_environmentparameter removed.Also applies to: 1952-1954
Contributor
Author
|
/verified |
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:
Your deliverable: Test Execution Plan
Guidelines:
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: |
openshift-virtualization-qe-bot-4
added
lgtm-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3528 🏁 Script executed: Length of output: 1809 🏁 Script executed: Length of output: 2239 🏁 Script executed: Length of output: 1781 🧠 Learnings used |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 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.
Short description:
Add admin_client to download_file_from_cluster
More details:
Add specific admin client instead of calling get_client()
What this PR does / why we need it:
Remove uses of get_client()
jira-ticket:
https://issues.redhat.com/browse/CNV-73718
Summary by CodeRabbit
Refactor
Impact
✏️ Tip: You can customize this high-level summary in your review settings.