-
Notifications
You must be signed in to change notification settings - Fork 165
[WIP] Updating CRUD operations test for automate method #8876
base: master
Are you sure you want to change the base?
Conversation
c8d1e4b
to
cb9e527
Compare
cb9e527
to
4c5c16e
Compare
fe04e8b
to
8f0240e
Compare
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.
A few questions, overall looking really nice! 👍
cfme/automate/explorer/method.py
Outdated
return self.parent_obj.tree_path + [ | ||
(icon_name_map[self.location.lower()], '{} ({})'.format(self.display_name, | ||
self.name))] | ||
if self.location in ['Ansible Tower Job Template', 'Ansible Tower Workflow Template']: |
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 making this strings class level or module level attributes? That way if they need to be updated, you will only have to update them there.
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 John,
- I am not getting what you actually want here. Do you want me to create global list of locations?
- Please explain more and give me example.
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.
@ganeshhubale sorry for delayed reply, I must've missed this. Yeah, I was suggesting that you make global
list lof locations, something like:
LOCATIONS = ['Ansible Tower Job Template', 'Ansible Tower Workflow Template']
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.
Got it @john-dupuy
I will update with this change.
8f0240e
to
9287043
Compare
1297b8d
to
cf1c4a4
Compare
b9ac4b1
to
39c021f
Compare
39c021f
to
874d1af
Compare
874d1af
to
567807f
Compare
567807f
to
4db6bfe
Compare
4db6bfe
to
6d8bb3f
Compare
cfme/automate/explorer/method.py
Outdated
}) | ||
validate = False | ||
|
||
if location == 'Ansible Tower Job Template': |
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 don't see any difference in add_page.fill for both the locations , why do we need if
checks ?
0121113
to
36b5905
Compare
@@ -484,6 +507,15 @@ def create( | |||
'playbook_input_parameters': playbook_input_parameters | |||
}) | |||
validate = False | |||
|
|||
if location in ['Ansible Tower Workflow Template', 'Ansible Tower Job Template']: |
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.
if location in self.LOCATIONS:
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 location
is fine there, no need for self
as location
is an argument in create.
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.
Yup that was a typo :)
'method_display_name': display_name, | ||
'ansible_max_ttl': max_ttl, | ||
}) | ||
validate = 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.
Why are you setting validate=false here ?
Don't we need to validate after fillng data ?
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.
@sshveta For method types such as 'Ansible Tower Workflow Template', 'Ansible Tower Job Template', 'playbook'
; we don't have such feature with button validate. This is applicable only for inline type of method.
) | ||
|
||
|
||
class Method(BaseEntity, Copiable): | ||
|
||
LOCATIONS = ['Ansible Tower Job Template', 'Ansible Tower Workflow Template'] |
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 making it global so that you can import it in test file too ?
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.
One question, thanks for this PR @ganeshhubale
save_button = Button('Save') | ||
reset_button = Button('Reset') | ||
cancel_button = Button('Cancel') | ||
|
||
def before_fill(self, values): | ||
location = self.context['object'].location.lower() | ||
if 'display_name' in values and location in ['inline', 'playbook']: | ||
values['{}_display_name'.format(location)] = values['display_name'] | ||
if 'display_name' in values: |
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.
@ganeshhubale What about the playbook
case? Won't this fail if location=='playbook'
?
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 @john-dupuy
Actually I am not getting why this question. So I tested test_automate_ansible_playbook_method_type_crud < this test case with these changes locally. And it works as expected.
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.
@ganeshhubale I have some test that uses this method that has failed in this section of code before. I remember I had to add in the location in ['inline', 'playbook']
line to get it to work. Can you please try running test_alert_run_ansible_playbook
?
My point is that you've removed the code that handles the case where location == 'playbook'
. I.e. to keep the original code intact, L324 should be
if location in ['inline', 'playbook']:
Does that make sense? Or am I missing something here?
elif 'name' in values and location in ['inline', 'playbook']: | ||
values['{}_name'.format(location)] = values['name'] | ||
|
||
elif 'name' in values: |
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.
Same question here about playbook
case.
Signed-off-by: Ganesh Hubale <[email protected]>
36b5905
to
554ce24
Compare
I detected some fixture changes in commit 554ce24 The local fixture
Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃 |
Would you mind rebasing this Pull Request against latest master, please? |
PRT RUN
{{ pytest: cfme/tests/automate/test_method.py::test_method_crud -vv }}