[1LP][RFR] Rewrite of test_infrastructure_hosts_crud for only single hosts

[prichard77]

Purpose or Intent

• Updating tests for infra hosts CRUD to make coverage more intuitive and manageable.

PRT Run

{{ pytest: --use-provider complete --long-running cfme/tests/infrastructure/test_host.py::test_infrastructure_hosts_crud }}

[prichard77]

I can add a wait and then check that a host is deleted. This will add a bit of time to the test.

[prichard77]

I moved the original crud test to PR#9762 where I will be adding additional coverage.

[mshriver]

I'd use fauxfactory to get a random alphanumeric here, not rely on datetime.

[prichard77]

Sure, I updated to use fauxfactory. I am just curious as to why this is preferred to datetime. Is it just because datetime will take more processing (CPU cycles)?

[mshriver]

I would assert that this view is displayed, if its the expected redirect after clicking save. You can pass wait=5 to create_view to trigger it calling wait_displayed() on the view class.

[prichard77]

Updated as suggested.

[mshriver]

see comment

[mshriver]

Please insert a line of whitespace before these comments so they're more visible blocks of the test function.

I would also explicitly navigate here, not rely on just creating the view from the prior assumption of redirection.

Tough call here on reducing the code duplication involved in filling/saving the form when there are only 2-3 things different between collection modify and details modify. Some test functions parametrize on this (from_details, from_collection)

One nuance here - the Host entity class includes Updateable - and the changes that you're making to the entity via these direct calls to navigation destinations and direct form filling are not being included in the actual entity.

This can cause issues with subsequent navigation - as the navigation steps may rely on the fields that have been modified in CFME.

Consider Host.exists uses a navigate to 'Details', and the step for navigating to details picks the entity in the table based on its custom_ident field.

If you modify that field in CFME, but still have the original instance of the host entity, this call will fail as CFME now has a different value than the entity in python.

In order to avoid this, updateable provides a context manager that updates the entity instance.

from cfme.utils.update import update

host = appliance.collections.hosts.all()[0]
new_value = fauxfactory.gen_alphanumeric()

with update(host):
    host.custom_ident = new_value

assert host.custom_ident == new_value
assert navigate_to(host, 'Details').entities.summary("Properties").get_text_of("Custom Identifier") == new_value

# both the entity instance and CFME have the same value.
# update context has taken care of updating the instance attributes, and has called edit on the instance.

Now, looking in detail at the Host entity code - its using the name attribute for navigation and identification, not custom_ident, so you should be okay with them not matching.

The update method also uses the entity's 'Edit' navigation destination, which only navigates via details page.

The choice/decision here is:

1. Do you want to keep very explicit navigation/fill calls in the test, kind of decoupling the test from the framework methods, and making it require direct updates instead of updates in the framework when components of MIQ change.

2. Update the framework methods/classes to provide: 'Edit' navigation via collection, allowing you to pass a kwarg through the update context manager to control how update executes.

I would advocate for choice #2, and add a kwarg to Host.update, maybe 'from_details' that takes a boolean and controls how it navigates to the edit form.

This would also allow you to move the flash message assertions out of the Host.update method, following our best practice for flash message checking in CRUD methods.

Then your test can be written like the above code example - just passing an extra kwarg to update context.

with update(host, from_detail=test_param_indicating_nav_path):
    host.custom_ident = something_random

[prichard77]

of course choice #2 is the way to go. Working on it now.

[prichard77]

Updated as I believe you were suggesting. I am still working on a nav bug causing nav to details before select Edit even when testing from hosts, but wanted to get comments in the mean time.

[prichard77]

I still have a bug with navigation to work out for a specific case, but wanted to keep the review going.

[john-dupuy]

1 small change, LGTM otherwise!

[john-dupuy]

can you move this to the top of the file?

[prichard77]

Done

[prichard77]

I included some additions to HostEditView that ended up not being used in my final implementation. I figured I'd leave them in as they have been tested and will probably be needed sometime in the future. I added a cancel button and is_displayed. This is ready for review again.

[prichard77]

1 change is requested from Mike. I have already made these updates. Mike, please let me know if I missed something.

[mshriver]

Don't trust the update context manager I see... :)