Alter API Ping usage in tests to match new endpoint#20909
Alter API Ping usage in tests to match new endpoint#20909pondrejk merged 2 commits intoSatelliteQE:masterfrom
Conversation
Reviewer's GuideUpdate Foreman ping-related tests to reflect the new API ping response structure, focusing on Katello-specific results instead of the legacy flat services/status layout. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In multiple places you now repeat the long indexing chain
target_sat.api.Ping().search_json()['results']['katello']['services']['candlepin_events']; consider introducing a small helper (e.g.get_katello_ping(target_sat)) or at least a local variable to holdresponse['results']['katello']to reduce duplication and improve readability. - The tests now assume that
response['results']['katello']is always present and correctly shaped; if this can vary between environments or versions, you might want to add a small guard or a clearer failure message when that structure is missing to make debugging easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In multiple places you now repeat the long indexing chain `target_sat.api.Ping().search_json()['results']['katello']['services']['candlepin_events']`; consider introducing a small helper (e.g. `get_katello_ping(target_sat)`) or at least a local variable to hold `response['results']['katello']` to reduce duplication and improve readability.
- The tests now assume that `response['results']['katello']` is always present and correctly shaped; if this can vary between environments or versions, you might want to add a small guard or a clearer failure message when that structure is missing to make debugging easier.
## Individual Comments
### Comment 1
<location path="tests/foreman/destructive/test_ping.py" line_range="60" />
<code_context>
"""
response = tomcat_service_teardown.api.Ping().search_json()
- assert response['status'] == 'FAIL'
+ assert response['results']['katello']['status'] == 'FAIL'
</code_context>
<issue_to_address>
**issue (testing):** Align assertion with the test’s "API ping fails" expectation by also checking the global failure indicator
Previously this test asserted the top-level `response['status'] == 'FAIL'`, matching the docstring "API ping fails and shows FAIL status." It now only checks `response['results']['katello']['status']`. If the new API still exposes a global failure indicator (e.g. a top-level `status`), please assert both the global status and the Katello status so the test verifies a full API ping failure, not just Katello. If the global flag no longer exists, update the docstring to state that this test now only validates the Katello section’s failure state.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| """ | ||
| response = tomcat_service_teardown.api.Ping().search_json() | ||
| assert response['status'] == 'FAIL' | ||
| assert response['results']['katello']['status'] == 'FAIL' |
There was a problem hiding this comment.
issue (testing): Align assertion with the test’s "API ping fails" expectation by also checking the global failure indicator
Previously this test asserted the top-level response['status'] == 'FAIL', matching the docstring "API ping fails and shows FAIL status." It now only checks response['results']['katello']['status']. If the new API still exposes a global failure indicator (e.g. a top-level status), please assert both the global status and the Katello status so the test verifies a full API ping failure, not just Katello. If the global flag no longer exists, update the docstring to state that this test now only validates the Katello section’s failure state.
|
vsedmik
left a comment
There was a problem hiding this comment.
One proposal and tiny request for change, otherwise looks good!
|
PRT Result |
With the API endpoint changed from
katello/api/v2/pingtoapi/v2/pingin SatelliteQE/nailgun#1406 (read the reasoning for the change in nailgun PR), some tests need updating.Summary by Sourcery
Update API ping tests to align with the new ping endpoint response structure focused on Katello-specific results.
Bug Fixes:
Enhancements:
Tests: