-
Notifications
You must be signed in to change notification settings - Fork 165
[1LP][WIP] Test Ansible Tower job status #9349
base: master
Are you sure you want to change the base?
Conversation
e17644b
to
3145dcd
Compare
b49401b
to
790bb2b
Compare
790bb2b
to
eaab74c
Compare
I detected some fixture changes in commit eaab74cb3382bfd7a4e1c049cfca93a11d6b9d74 The global fixture
Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃 |
eaab74c
to
1ff8627
Compare
1ff8627
to
dabb298
Compare
60ba3f3
to
c283e41
Compare
msg = "Ansible Tower Job failed" | ||
assert tower_job.is_job_successful(), msg | ||
|
||
if tower_job.exists: |
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.
Thoughts on moving this into a request.addfinalizer
block? Then using tower_job.delete_if_exists()
?
cfme/ansible_tower/jobs.py
Outdated
view.flash.assert_no_error() | ||
|
||
def is_job_successful(self): | ||
return True if self.status == "successful" else False |
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.
No need for a ternary selecting booleans around a statement that produces a boolean.
return self.status == 'successful'
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.
I would also make this a parameter, and simply call it successful
def is_job_successful(self): | ||
return True if self.status == "successful" else False | ||
|
||
def wait_for_completion(self, num_sec=1200, delay=10): |
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.
Can we lower this default delay? 10s is quite slow.
|
||
def wait_for_completion(self, num_sec=1200, delay=10): | ||
def last_status(): | ||
logger.info("Last status message in UI: '{}'".format(self.status)) |
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.
Is there ever a refresh in this? There is navigation to Details page here and in calling is_job_successful, but if its never going away from that page its relying on a resetter method to be refreshing.
def status(self, template_name): | ||
"""Get status of a specific job from the All page.""" | ||
view = navigate_to(self, 'All') | ||
for e in view.entities.get_all(surf_pages=True): |
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.
There's a more efficient way to do this - use EntitiesConditionalView.get_entity(surf_pages=True, template_name=template_name)
to get the specific entity without iterating through all of them.
Is it possible more than one will have the same template_name? If so you may want to use get_entities_by_keys()
directly.
|
||
def delete_all(self): | ||
view = navigate_to(self, 'All') | ||
view.paginator.check_all() |
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.
I believe this is only deleting all the entities on the current page.
Mixed thoughts on a convenience method like this being added to a collection.
A far more useful implementation would take a list of entities to select and delete, with an 'all' kwarg to handle selection via paginator. Combined with standardized definition of the configuration string used to delete the entities, this could be implemented in BaseCollection.
I don't see any uses of this method in your current tests, were you adding it for future use? Perhaps a large-scope fixture that could cleanup everything after a tower test module runs?
|
||
@property | ||
def are_all_jobs_successful(self): | ||
return all([self.is_job_successful(job.template_name) for job in self.all()]) |
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.
Again I'm questioning the implementation and use of such a convenience method on the collection.
This isn't taking advantage of the fact that the status can be read from the collection page - its actually instantiating entities and using their status properties, navigating to every entity in turn. I think its very misleading as a collection method.
I don't see its use anywhere here, and am doubting that its necessary for test automation.
caseimportance: medium | ||
initialEstimate: 1/4h | ||
""" | ||
template = config_manager_obj.yaml_data['provisioning_data'][job_type] |
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.
I think this needs a wrapper so the test doesn't end with an ERROR in the Call phase because the yaml is missing a job type.
It should skip based on the KeyError that this statement will produce.
cells = {'Description': catalog_item.name} | ||
order_request = appliance.collections.requests.instantiate(cells=cells, partial_check=True) | ||
order_request.wait_for_request(method='ui') | ||
msg = "Request failed with the message {}".format(order_request.row.last_message.text) |
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.
Consider using f-strings
tower_job = appliance.collections.ansible_tower_jobs.instantiate(template_name=template) | ||
request.addfinalizer(tower_job.delete_if_exists) | ||
tower_job.wait_for_completion() | ||
msg = "Ansible Tower Job failed" |
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.
No need to name this, just pass it in-line below :)
def is_job_successful(self): | ||
return True if self.status == "successful" else False | ||
|
||
def wait_for_completion(self, num_sec=1200, delay=10): |
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.
This is actually waiting for successful completion, because its relying on is_job_successful
. Either it needs to renamed, or re-written to look for multiple states. It would be good to enumerate those possible states on the entity class so that they can be referenced consistently.
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.
Several things to address with the proposed convenience methods. Thanks Nandini!
Would you mind rebasing this Pull Request against latest master, please? |
Purpose or Intent
1)Updated TowerJobs and TowerJobsCollection on the Tower Jobs page .
2)Added a test to check the status of Tower Jobs
PRT Run
{{py.test: cfme/tests/services/test_config_provider_servicecatalogs.py -k "status"}}
PRT results:
510 - PASS
511 - PASS