-
Notifications
You must be signed in to change notification settings - Fork 131
[6.18.z] update the page old to new UI #20528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Satellite-QE
wants to merge
1
commit into
6.18.z
Choose a base branch
from
cherry-pick-6.18.z-34b039829be117b2fdb49207538287f830f84c02
base: 6.18.z
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
[6.18.z] update the page old to new UI #20528
Satellite-QE
wants to merge
1
commit into
6.18.z
from
cherry-pick-6.18.z-34b039829be117b2fdb49207538287f830f84c02
+54
−34
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
Collaborator
Author
|
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- In
test_computeresource_gcethe second finalize block still usessession.host.search(hostname)while other calls were migrated tosession.host_new, which is inconsistent with the rest of the UI migration and may be pointing to the old page inadvertently. - Several tests now access deeply nested structures like
host_page['overview']['details']['details'][f'{settings.server.NETWORK_TYPE}_address']; consider adding a small helper to retrieve common fields (e.g., host group, IP address, status) from the new UI payload to avoid repetition and reduce brittleness to structural changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_computeresource_gce` the second finalize block still uses `session.host.search(hostname)` while other calls were migrated to `session.host_new`, which is inconsistent with the rest of the UI migration and may be pointing to the old page inadvertently.
- Several tests now access deeply nested structures like `host_page['overview']['details']['details'][f'{settings.server.NETWORK_TYPE}_address']`; consider adding a small helper to retrieve common fields (e.g., host group, IP address, status) from the new UI payload to avoid repetition and reduce brittleness to structural changes.
## Individual Comments
### Comment 1
<location> `tests/foreman/ui/test_discoveredhost.py:289` </location>
<code_context>
@pytest.mark.on_premises_provisioning
@pytest.mark.parametrize('module_provisioning_sat', ['discovery'], indirect=True)
[email protected]('pxe_loader', ['bios', 'uefi'], indirect=True)
[email protected]('pxe_loader', ['bios'], indirect=True)
@pytest.mark.rhel_ver_match('9')
def test_positive_auto_provision_host_with_rule(
</code_context>
<issue_to_address>
**issue (testing):** Dropping `uefi` from the parametrized `pxe_loader` reduces test coverage of the provisioning path
This change means the test no longer validates discovery + auto‑provisioning for UEFI. If this is due to flakiness or a UI limitation, consider either keeping `uefi` and updating the assertions, or clearly documenting why `uefi` is excluded and adding a separate UEFI‑specific test to preserve coverage of that path.
</issue_to_address>
### Comment 2
<location> `tests/foreman/ui/test_computeresource_gce.py:210` </location>
<code_context>
- assert host_info['properties']['properties_table']['Build'] == 'Installed clear'
+ host_info = session.host_new.get_host_statuses(hostname)
+ assert session.host_new.search(hostname)[0]['Name'] == hostname
+ assert host_info['Build']['Status'] == 'Installed'
# 1.2 GCE Backend Assertions
gceapi_vm = googleclient.get_vm(gceapi_vmname)
</code_context>
<issue_to_address>
**suggestion (testing):** New UI assertions no longer verify the "clear"/build flag that the old tests were checking
Previously we asserted `host_info['properties']['properties_table']['Build'] == 'Installed clear'`, which validated both install status and that the build flag was cleared. The new check `host_info['Build']['Status'] == 'Installed'` only covers install status. If the new API exposes an equivalent “clear”/build-complete indicator (e.g., another field), please add an assertion for it so we preserve the original test intent.
Suggested implementation:
```python
host_info = session.host_new.get_host_statuses(hostname)
assert session.host_new.search(hostname)[0]['Name'] == hostname
assert host_info['Build']['Status'] == 'Installed'
# Preserve original intent: verify build flag is cleared/completed in addition to install status
assert host_info['Build']['Clear'] is True
# 1.2 GCE Backend Assertions
```
You’ll need to adjust `host_info['Build']['Clear']` to match the actual field the new API exposes for the “clear”/build-complete indicator. For example, depending on the schema this might instead be something like:
- `host_info['Build']['Flag'] == 'clear'`
- `host_info['Build']['Completed'] is True`
- `host_info['Build']['StatusDetail'] == 'Installed clear'`
Please inspect the structure returned by `session.host_new.get_host_statuses(hostname)` (e.g., via debugging/logging in another test or REPL) and update the assertion to match the correct key and expected value.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Collaborator
Author
|
PRT Result |
Member
|
Collaborator
Author
|
PRT Result |
Member
|
Collaborator
Author
|
PRT Result |
dcc470d to
64d0ac8
Compare
update the host page old to new UI (cherry picked from commit 34b0398)
64d0ac8 to
c731424
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
6.18.z
Introduced in or relating directly to Satellite 6.18
Auto_Cherry_Picked
Automatically cherrypicked PR using GHA
AutoMerge_Cherry_Picked
The cherrypicked PRs of master PR would be automerged if all checks passing
No-CherryPick
PR doesnt need CherryPick to previous branches
PRT-Failed
Indicates that latest PRT run is failed for the PR
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.
Cherrypick of PR: #20370
Updated the code in some places so that it now loads the new UI page instead of the old UI for Rocket-based components.
Dependent PR: SatelliteQE/airgun#2231