Skip to content
This repository has been archived by the owner on Apr 7, 2022. It is now read-only.

[1LP][RFR] Add test_infrastructure_hosts_viewing #10197

Merged
merged 13 commits into from
Jun 29, 2020

Conversation

prichard77
Copy link
Contributor

@prichard77 prichard77 commented Jun 18, 2020

Purpose or Intent

  • Adding test test_infrastructure_hosts_viewing to cover manual, test_host_viewing

PRT Run

{{ pytest: cfme/tests/infrastructure/test_host.py::test_infrastructure_hosts_viewing }}

@prichard77 prichard77 changed the title [WIP] Add test_infrastructure_hosts_viewing [WIPTEST] Add test_infrastructure_hosts_viewing Jun 18, 2020
@prichard77 prichard77 changed the title [WIPTEST] Add test_infrastructure_hosts_viewing [RFR] Add test_infrastructure_hosts_viewing Jun 19, 2020
Copy link
Contributor

@john-dupuy john-dupuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome use of parametrization @prichard77 -- nice test case! Couple of small questions and the manual file needs rebase I think.

@john-dupuy john-dupuy added the test-automation To be applied on PR's which are automating existing manual cases label Jun 19, 2020
@john-dupuy john-dupuy changed the title [RFR] Add test_infrastructure_hosts_viewing [WIPTEST] Add test_infrastructure_hosts_viewing Jun 19, 2020
@prichard77 prichard77 changed the title [WIPTEST] Add test_infrastructure_hosts_viewing [RFR] Add test_infrastructure_hosts_viewing Jun 23, 2020
@prichard77 prichard77 requested a review from john-dupuy June 23, 2020 13:33
Copy link
Contributor

@john-dupuy john-dupuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small comment, otherwise LGTM!

@@ -15,14 +15,20 @@
from cfme.common.candu_views import HostInfraUtilizationView
from cfme.common.host_views import HostAddView
from cfme.common.host_views import HostDetailsView
from cfme.common.host_views import HostDevicesView
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be time to just import the whole module...... optional though, and could get you into some circular imports.

@john-dupuy john-dupuy changed the title [RFR] Add test_infrastructure_hosts_viewing [1LP][RFR] Add test_infrastructure_hosts_viewing Jun 24, 2020
@test_requirements.infra_hosts
@pytest.mark.parametrize("table,row,view", HOST_DETAIL_ROWS,
ids=[rel[1] for rel in HOST_DETAIL_ROWS])
def test_infrastructure_hosts_viewing(appliance, setup_provider, host, table, row, view):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of the table parameter here? The navigation destinations select this automatically, I think it only would have been necessary if you were interacting with the summary table widgets directly within the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. I forgot to remove when I created the navigation. Removing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But wait. I used this to check that the value was not '0'. Might need to keep it.

@mshriver mshriver self-assigned this Jun 24, 2020
@prichard77 prichard77 requested a review from mshriver June 24, 2020 19:38
Comment on lines 568 to 574
if row not in ["Operating System", "Networks", "Print or export"]:
view = navigate_to(host, "Details")
pytest.skip(f"No row item present for {row}")
view = navigate_to(host, row)
if row == "Print or export":
handle_extra_tabs(view)
assert view.is_displayed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This got a bit screwed up by the suggestion merged as 333b964

Suggested change
if row not in ["Operating System", "Networks", "Print or export"]:
view = navigate_to(host, "Details")
pytest.skip(f"No row item present for {row}")
view = navigate_to(host, row)
if row == "Print or export":
handle_extra_tabs(view)
assert view.is_displayed
@request.addfinalizer
def _finalizer():
if row == "Print or export":
handle_extra_tabs(view)
try:
if row not in ["Operating System", "Networks", "Print or export"]:
view = navigate_to(host, "Details")
if view.entities.summary(table).get_text_of(row) == '0':
pytest.skip(f"No items present for host relationship {row}")
view = navigate_to(host, row)
except TimedOutError:
pytest.fail('Timed out navigating to host relationship {row}')

@mshriver mshriver changed the title [1LP][RFR] Add test_infrastructure_hosts_viewing [1LP][WIPTEST] Add test_infrastructure_hosts_viewing Jun 24, 2020
@prichard77
Copy link
Contributor Author

I'll get some clarification in sync on Thursday. I could implement what I believe you are saying in chat by also adding an explicit click just before the assertion, but this doesn't make much sense to me. If navigating_to() the destination doesn't change the view then it would fail on is_displayed before I could assert that the original view is_displayed. I must be misunderstanding something about what you are asking.

I'd just change that pytest.skip to an assert that the details view is still displayed basically assert that navigating to the destination doesn't change the view

@dajoRH
Copy link
Contributor

dajoRH commented Jun 25, 2020

I detected some fixture changes in commit 3233094

The local fixture host is used in the following files:

  • cfme/tests/infrastructure/test_host.py
    • test_infrastructure_hosts_viewing

Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃

@prichard77 prichard77 changed the title [1LP][WIPTEST] Add test_infrastructure_hosts_viewing [1LP][RFR] Add test_infrastructure_hosts_viewing Jun 25, 2020
@prichard77 prichard77 requested a review from mshriver June 25, 2020 21:31
Copy link
Member

@mshriver mshriver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work asserting against the redirection (or lack thereof) behavior all within one test!

@mshriver mshriver merged commit b8f53d5 into ManageIQ:master Jun 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
test-automation To be applied on PR's which are automating existing manual cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants