test(RHINENG-20830): Re-use uploaded hosts in multiple IQE tests #3317
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
Contributor
Reviewer's GuideRefactors multiple IQE host-inventory REST tests to reuse pre-created hosts via shared fixtures, removes redundant OS parametrization and system-profile assertions where not needed, and tightens a few test assertions around org_id and staleness timestamps. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
prepare_hostshelper intest_api_patch.pyis decorated with@pytest.mark.fixtureinstead of@pytest.fixture, which will prevent pytest from registering it as a fixture; this should be corrected. - Several tests now share the same module-scoped hosts via
prepare_hosts(e.g., intest_api_patch.pyand kessel tests); double-check that each test either treats these hosts as immutable or fully accounts for prior mutations (like patched fields) to avoid hidden inter-test dependencies and flakiness.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `prepare_hosts` helper in `test_api_patch.py` is decorated with `@pytest.mark.fixture` instead of `@pytest.fixture`, which will prevent pytest from registering it as a fixture; this should be corrected.
- Several tests now share the same module-scoped hosts via `prepare_hosts` (e.g., in `test_api_patch.py` and kessel tests); double-check that each test either treats these hosts as immutable or fully accounts for prior mutations (like patched fields) to avoid hidden inter-test dependencies and flakiness.
## Individual Comments
### Comment 1
<location> `iqe-host-inventory-plugin/iqe_host_inventory/tests/rest/test_api_patch.py:20-21` </location>
<code_context>
logger = logging.getLogger(__name__)
[email protected](scope="module")
+def prepare_hosts(host_inventory: ApplicationHostInventory) -> list[HostOut]:
+ return host_inventory.upload.create_hosts(2, cleanup_scope="module")
+
</code_context>
<issue_to_address>
**issue (testing):** The fixture decorator is incorrect and will prevent the fixture from being registered.
Using `@pytest.mark.fixture(scope="module")` will not register `prepare_hosts` as a fixture, so any tests that use it will fail with `FixtureNotFound` during collection. Please change this decorator to `@pytest.fixture(scope="module")`.
</issue_to_address>
### Comment 2
<location> `iqe-host-inventory-plugin/iqe_host_inventory/tests/rest/kessel/test_kessel_groups.py:28-29` </location>
<code_context>
logger = logging.getLogger(__name__)
[email protected](scope="module")
+def prepare_hosts(host_inventory: ApplicationHostInventory) -> list[HostOut]:
+ return host_inventory.upload.create_hosts(3, cleanup_scope="module")
+
</code_context>
<issue_to_address>
**suggestion (testing):** Module-scoped `prepare_hosts` is shared across multiple group tests that mutate group membership, causing potential test ordering issues.
Because this fixture is module-scoped and shared by tests that mutate group membership for the same hosts, test behavior can depend on execution order, leading to flaky or order-dependent tests.
Please either make this fixture function-scoped so each test gets fresh hosts, or introduce a factory fixture (e.g. `prepare_hosts_factory()`) that creates hosts per test. This will keep the group-related tests isolated and predictable.
Suggested implementation:
```python
@pytest.fixture(scope="function")
def prepare_hosts(host_inventory: ApplicationHostInventory) -> list[HostOut]:
return host_inventory.upload.create_hosts(3, cleanup_scope="function")
```
If there are any tests that rely on host state persisting across tests (within the module), they may need to be adjusted to either:
1. Use their own dedicated fixture with broader scope, or
2. Explicitly coordinate state using API calls instead of relying on shared `prepare_hosts` instances.
</issue_to_address>
### Comment 3
<location> `iqe-host-inventory-plugin/iqe_host_inventory/tests/rest/kessel/test_kessel_ungrouped.py:25-26` </location>
<code_context>
logger = logging.getLogger(__name__)
[email protected](scope="module")
+def prepare_hosts(host_inventory: ApplicationHostInventory) -> list[HostOut]:
+ return host_inventory.upload.create_hosts(3, cleanup_scope="module")
+
</code_context>
<issue_to_address>
**suggestion (testing):** Shared module-scoped hosts in ungrouped tests can cause cross-test interference.
Here, `prepare_hosts` is module-scoped and reused by `test_kessel_filter_ungrouped_hosts` and `test_kessel_get_hosts_ordering_with_ungrouped`, both of which change grouping on the same hosts. This can make the second test observe hosts already grouped by the first.
Please switch this fixture to function scope or make it a factory that creates fresh hosts per test to avoid order-dependent/flaky behavior.
Suggested implementation:
```python
@pytest.fixture
def prepare_hosts(host_inventory: ApplicationHostInventory) -> list[HostOut]:
return host_inventory.upload.create_hosts(3, cleanup_scope="function")
```
1. Ensure `pytest` is imported in this module (e.g. `import pytest`) if it is not already present at the top of the file.
2. No test changes should be needed, as the fixture name and signature remain the same; the behavior will now provide fresh hosts per test.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
iqe-host-inventory-plugin/iqe_host_inventory/tests/rest/test_api_patch.py
Outdated
Show resolved
Hide resolved
iqe-host-inventory-plugin/iqe_host_inventory/tests/rest/kessel/test_kessel_groups.py
Show resolved
Hide resolved
iqe-host-inventory-plugin/iqe_host_inventory/tests/rest/kessel/test_kessel_ungrouped.py
Show resolved
Hide resolved
fstavela
force-pushed
the
fstavela/reuse-hosts-in-iqe
branch
from
caded57 to
ca90b85
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Overview
This PR is being created to address RHINENG-20830.
PR Checklist
Secure Coding Practices Documentation Reference
You can find documentation on this checklist here.
Secure Coding Checklist
Summary by Sourcery
Refactor host inventory IQE tests to reuse uploaded hosts across multiple cases and tighten verification of host metadata and staleness timestamps.
Bug Fixes:
Enhancements:
Tests: