feat(RHINENG-24459): Add IQE e2e test cases for sparse field inventory views endpoint#3710
feat(RHINENG-24459): Add IQE e2e test cases for sparse field inventory views endpoint#3710adarshdubey-star wants to merge 10 commits intoRedHatInsights:masterfrom
Conversation
Reviewer's GuideAdds a new IQE integration test module that validates JSON:API sparse fieldsets behavior for the /beta/hosts-view endpoint’s app_data, covering single/multi-app selections, the app_data meta-key, edge cases, filtering interactions, per-app coverage, and default behavior when the fields parameter is omitted. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
SC Environment Impact AssessmentOverall Impact: ⚪ NONE No SC Environment-specific impacts detected in this PR. What was checkedThis PR was automatically scanned for:
|
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- There’s quite a bit of repeated setup logic for creating a host and then producing additional app data (e.g., advisor + vulnerability + patch/remediations across several classes); consider extracting common helpers/fixtures (like
setup_host_with_multiple_apps(...)or small builder functions forfieldsquery strings) to reduce duplication and make the intent of each test clearer. - In
TestSparseFieldsWithFiltering.test_sparse_fields_with_filter, you only assert thathost1is included andhost2is excluded; it would be more robust to additionally assert that every host inresponse.resultssatisfies the[advisor][recommendations][gte]=5condition to fully verify the filter behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There’s quite a bit of repeated setup logic for creating a host and then producing additional app data (e.g., advisor + vulnerability + patch/remediations across several classes); consider extracting common helpers/fixtures (like `setup_host_with_multiple_apps(...)` or small builder functions for `fields` query strings) to reduce duplication and make the intent of each test clearer.
- In `TestSparseFieldsWithFiltering.test_sparse_fields_with_filter`, you only assert that `host1` is included and `host2` is excluded; it would be more robust to additionally assert that every host in `response.results` satisfies the `[advisor][recommendations][gte]=5` condition to fully verify the filter behavior.
## Individual Comments
### Comment 1
<location path="iqe-host-inventory-plugin/iqe_host_inventory/tests/rest/test_host_views_sparse_fields.py" line_range="216-225" />
<code_context>
+class TestSparseFieldsMetaKey:
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for combined usage of `fields[app_data]=true` with app-specific `fields[app]=...`
`TestSparseFieldsMetaKey` currently only covers the bare `fields[app_data]=true` case. Please add a test that also requests an app-specific field, e.g. `fields[app_data]=true&fields[advisor]=recommendations`, and asserts the intended precedence/merge behavior (does `app_data=true` imply all advisor fields, or are we limited to `recommendations`?). This will lock in the contract for how meta-keys and app-specific field requests interact.
Suggested implementation:
```python
class TestSparseFieldsMetaKey:
"""Test the fields[app_data]=true meta-key behavior."""
def test_app_data_true_returns_all_apps(
self,
host_inventory: ApplicationHostInventory,
setup_host_with_app_data,
):
"""fields[app_data]=true returns all available app data.
This meta-key should cause app_data to be included for all apps that have
stored data, without any per-app sparse-field restriction.
"""
# Arrange: ensure a host exists with advisor and vulnerability app data
host = setup_host_with_app_data
# Act: request the host with the meta-key only
response = host_inventory.api_get(
f"/api/inventory/v1/hosts/{host.id}",
params={"fields[app_data]": "true"},
)
response.raise_for_status()
result = response.json()["results"][0]
# Assert: all apps under app_data are present, and all their fields are
# available (no sparse-field limitation per app).
advisor_data = result["app_data"]["advisor"]
vuln_data = result["app_data"]["vulnerability"]
# We expect both apps to be present
assert advisor_data is not None
assert vuln_data is not None
# For advisor we expect at least both recommendations and incidents
# (exact numeric values depend on the fixture, but both keys must exist)
assert "recommendations" in advisor_data
assert "incidents" in advisor_data
# For vulnerability we expect at least the main CVE-related fields
assert "critical_cves" in vuln_data
assert "total_cves" in vuln_data
def test_app_data_true_with_app_specific_fields_uses_app_specific_filter(
self,
host_inventory: ApplicationHostInventory,
setup_host_with_app_data,
):
"""fields[app_data]=true combined with fields[advisor]=... behavior.
Contract:
- fields[app_data]=true turns on app_data for all apps that have data.
- A specific fields[advisor]=... clause still acts as a sparse-field
filter for the advisor app only.
- Other apps (e.g. vulnerability) remain unrestricted unless they have
their own fields[app]=... selector.
"""
# Arrange
host = setup_host_with_app_data
# Act: request app_data meta-key and a specific advisor field
response = host_inventory.api_get(
f"/api/inventory/v1/hosts/{host.id}",
params={
"fields[app_data]": "true",
"fields[advisor]": "recommendations",
},
)
response.raise_for_status()
result = response.json()["results"][0]
advisor_data = result["app_data"]["advisor"]
vuln_data = result["app_data"]["vulnerability"]
# Assert: advisor app is present but limited to the requested field(s)
assert advisor_data.get("recommendations") is not None
# The incidents field should be filtered out because of fields[advisor]
assert advisor_data.get("incidents") is None
# Assert: vulnerability app is present and not restricted by the
# advisor-specific sparse field selector
assert vuln_data.get("critical_cves") is not None
assert vuln_data.get("total_cves") is not None
```
The above changes assume the following, which you may need to align with your existing test helpers and fixtures:
1. There is an `api_get` helper on the `host_inventory` fixture that:
- Accepts a path and a `params` dict.
- Returns a `requests.Response`-like object with `.raise_for_status()` and `.json()`.
2. `setup_host_with_app_data` is a fixture that either returns a host object with an `.id` attribute or is itself such an object. If in your tests it *returns* the host instead of being the host, adjust:
- Replace `host = setup_host_with_app_data` with `host = setup_host_with_app_data()` (or the correct call pattern).
3. The response structure is assumed to be:
- `response.json()["results"][0]["app_data"]["advisor"]` and `"vulnerability"`.
If your helper already unwraps `"results"` and returns the host dict directly, you can simplify:
- `result = response.json()`
- `advisor_data = result["app_data"]["advisor"]`, etc.
4. If your existing tests use a different numeric contract (e.g. they assert specific values like 5, 2, 10), you can strengthen the new test assertions by replacing the `is not None` checks with those specific values to match the existing fixtures.
5. If `TestSparseFieldsMetaKey.test_app_data_true_returns_all_apps` already exists with a more specific implementation in your current file, you may want to merge its existing assertions into the updated version above rather than replace them wholesale.
</issue_to_address>
### Comment 2
<location path="iqe-host-inventory-plugin/iqe_host_inventory/tests/rest/test_host_views_sparse_fields.py" line_range="401-208" />
<code_context>
+ def test_sparse_fields_with_filter(
</code_context>
<issue_to_address>
**suggestion (testing):** Add a multi-app filtering scenario to ensure sparse fields and filters behave correctly with multiple app datasets
To more fully cover this behavior, please add a case where hosts have both `advisor` and `vulnerability` data, you filter on an `advisor` field, and you request only `vulnerability` fields via `fields[vulnerability]=...`. This will confirm that filtering logic is independent of sparse-field projection and that selecting `vulnerability` fields does not interfere with the `advisor` filter.
Suggested implementation:
```python
def test_sparse_fields_with_filter(
self,
host_inventory: ApplicationHostInventory,
setup_host_with_app_data,
):
"""Sparse fields work together with host filtering.
metadata:
requirements: inv-host-views-sparse-fields
assignee: adubey
importance: high
"""
# Baseline: sparse fields and filter on advisor data for a host with only advisor app data
host_only_advisor = setup_host_with_app_data(
host_inventory,
inventory_id="baseline-advisor-only",
advisor_data={"recommendations": 5, "incidents": 3},
)
response = host_inventory.get_hosts(
params={
"filter[advisor][recommendations]": "gt:0",
"fields[advisor]": "recommendations",
}
)
assert response.results, "Expected at least one host in baseline advisor-only scenario"
baseline_result = next(
r for r in response.results if r.id == host_only_advisor.id
)
advisor_data = baseline_result.app_data.advisor.to_dict()
# Only recommendations should be present due to sparse fields selection
assert advisor_data.get("recommendations") == 5
assert advisor_data.get("incidents") is None
# Multi-app scenario: host has both advisor and vulnerability data.
# We filter on advisor but request only vulnerability fields, to ensure the
# advisor filter is independent of sparse-field projection on vulnerability.
host_multi_app = setup_host_with_app_data(
host_inventory,
inventory_id="multi-app-host",
advisor_data={"recommendations": 4, "incidents": 1},
vulnerability_data={"cve_count": 10, "incidents": 2},
)
multi_app_response = host_inventory.get_hosts(
params={
# Filter is on advisor app data
"filter[advisor][recommendations]": "gt:0",
# Sparse fields selection is for vulnerability app data only
"fields[vulnerability]": "cve_count",
}
)
# Ensure the multi-app host is returned by the advisor filter
assert multi_app_response.results, "Expected hosts to match advisor filter in multi-app scenario"
multi_app_result = next(
r for r in multi_app_response.results if r.id == host_multi_app.id
)
# Confirm filtering logic is independent of sparse-field projection:
# - advisor filter worked (host is present)
# - vulnerability app data respects sparse fields
vulnerability_data = multi_app_result.app_data.vulnerability.to_dict()
assert vulnerability_data.get("cve_count") == 10
# Any vulnerability fields not requested should not be present (or be None)
assert vulnerability_data.get("incidents") is None
```
The above implementation assumes the following about your existing test harness:
1. `setup_host_with_app_data(host_inventory, inventory_id=..., advisor_data=..., vulnerability_data=...)`:
- Accepts `advisor_data` and `vulnerability_data` keyword arguments.
- Creates/updates a host with app data for the specified apps.
- Returns an object with an `id` attribute matching the IDs used in the `host_inventory.get_hosts(...)` results.
2. `host_inventory.get_hosts(params=...)`:
- Accepts a `params` dict that mirrors the REST query parameters (e.g. `filter[advisor][recommendations]`, `fields[vulnerability]`).
- Returns an object with a `results` attribute, which is an iterable of host result objects.
- Each result has:
- An `id` attribute.
- An `app_data` attribute with `advisor` and `vulnerability` attributes, each exposing a `.to_dict()` method.
If your fixtures or API helpers use different names or signatures (for example, `create_host_with_app_data` instead of `setup_host_with_app_data`, or `host_inventory.api_get` instead of `get_hosts`), please:
- Adjust the function calls (`setup_host_with_app_data`, `host_inventory.get_hosts`) to match your actual helpers.
- Map the `params` keys to however your test client currently passes query parameters into the REST calls.
</issue_to_address>
### Comment 3
<location path="iqe-host-inventory-plugin/iqe_host_inventory/tests/rest/test_host_views_sparse_fields.py" line_range="462-471" />
<code_context>
+class TestSparseFieldsAllApps:
</code_context>
<issue_to_address>
**suggestion (testing):** Extend per-app sparse-field tests to also assert non-requested fields are omitted/null
Since other tests already verify that non-requested fields are omitted or null, it would be helpful to extend this parametrized test to also assert that at least one known, non-requested field per app is absent or null in the response. That way we confirm sparse field projection is enforced across all supported apps, not just that requested fields are returned.
</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_host_views_sparse_fields.py
Show resolved
Hide resolved
iqe-host-inventory-plugin/iqe_host_inventory/tests/rest/test_host_views_sparse_fields.py
Show resolved
Hide resolved
iqe-host-inventory-plugin/iqe_host_inventory/tests/rest/test_host_views_sparse_fields.py
Outdated
Show resolved
Hide resolved
1dd4d98 to
4d372a9
Compare
|
/retest |
8ec6631 to
3a6238f
Compare
|
/retest |
iqe-host-inventory-plugin/iqe_host_inventory/tests/rest/test_host_views_sparse_fields.py
Outdated
Show resolved
Hide resolved
iqe-host-inventory-plugin/iqe_host_inventory/tests/rest/test_host_views_sparse_fields.py
Outdated
Show resolved
Hide resolved
kruai
left a comment
There was a problem hiding this comment.
I agree with Rodrigo's suggestions, but everything else looks good 👍
9148d23 to
0d18aa1
Compare
0d18aa1 to
4d47e58
Compare
|
/retest |
|
/retest |
|
👋 Hi, this PR has been inactive for 7 days. |
Adds comprehensive IQE integration tests for the
fieldsquery parameter (sparse fieldsets) on the/beta/hosts-viewendpoint.These tests verify the JSON: API sparse fieldsets implementation that allows the request of specific application data fields:
fields[app]=true- return all fields for that appfields[app]=field1,field2- return only specified fieldsfields[app]=- return empty object (per JSON:API spec)fields[app_data]=true- return all apps with all fieldsSummary by Sourcery
Add integration test coverage for JSON:API sparse fieldsets on the /beta/hosts-view endpoint, validating behavior of the fields query parameter across apps and scenarios.
Tests: