Skip to content
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

WebUI tests: test_service #1679

Closed
wants to merge 13 commits into from
Closed

Conversation

Rezney
Copy link
Collaborator

@Rezney Rezney commented Mar 14, 2018

Extension of test_service WebUI tests.

@pvoborni pvoborni added the re-run Trigger a new run of PR-CI label Mar 14, 2018
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Mar 14, 2018
@pvoborni
Copy link
Member

Michal asked me to do a quick check. These are my initial thoughts. I did not run the tests. I also did not read the commits which are not mentioned. Will get to it maybe later today.

ui_tests: select_combobox() fixes

LGTM

ui_tests: test cancel and delete without button

Would it work if we replace the confirm/cancel functionality with confirm_btn='ok'.

So in usage one could use:

delete_record(..., confirm_btn='cancel')

Because if I get it right then "confirm" option is there only to skip the click on 'ok' button and is is useful only together with cancel=True, right?

ui_tests: make associations cancelable

Same comment. If the self.wait_for_request() is a problem then we could make this part configurable(skippable)

ui_tests: add function to run cmd on UI host

run_cmd_on_ui_host we can specify what type of command to run. I.e. that it is a shell command and not e.g. a API command. I'm not sure if we should mention SSH in the comment. On one side it would be beneficial to get the meaning but it could then force some implementation and prevent future refactoring. Otherwise seems OK.

ui_tests: add funcs to add/remove users public SSH key

Looks good. Maybe I'd consider making some parts like navigation to entity and clicking on update buttons configurable, as somebody might already be on the page or would like to add more stuff. But this can be extended later when needed.

ui_tests: add assert_field_required()

Looks good. Maybe I'd just specify that where the error message is. E.g.

Assert we got 'Required field' error message in field validation

And maybe the command name could be something like:
assert_field_validation_required

As assert_field_required could also mean checking if field has '*' sign next to it.

ui_tests: add assert_notification()

Would it work if the find doesn't find it and assert_text is set. Seems to me that it may fail that None doesn't have attribute text.

ui_tests: run ipa-get/rmkeytab command on UI host

Would it work if run_keytab_on_host is run more than 1 time for different principals?

Also I'd rename pkey to principal and document possible actions in method comment.

@Rezney
Copy link
Collaborator Author

Rezney commented Mar 15, 2018

@pvoborni
Thanks for the first part of the review. Please see my actions upon it:

ui_tests: test cancel and delete without button:

Partially adjusted (confirm_btn), the other condition is needed for "key" testing.

ui_tests: make associations cancelable

Again adjusted only partially (confirm_btn). Later condition is needed in order to avoid assert error.

ui_tests: add function to run cmd on UI host

Fixed the docstring and also added a recommendation to use this sparsely.

ui_tests: add funcs to add/remove users public SSH key

Left this be for the moment as I will work with SSH keys in next suite.

ui_tests: add assert_field_required()

Fixed.

ui_tests: add assert_notification()

Fixed

ui_tests: run ipa-get/rmkeytab command on UI host

Changed to "pkey" to "principal".
Could you please elaborate more on your worries about running "run_keytab_on_host" more than 1 time for different principals?

Tried it with normal command and looks ok:

[root@master ~]# ipa-getkeytab -p dogtag/master.ipa.test@IPA.TEST -k /tmp/test.keytab Keytab successfully retrieved and stored in: /tmp/test.keytab [root@master ~]# ipa-getkeytab -p HTTP/master.ipa.test@IPA.TEST -k /tmp/test.keytab Keytab successfully retrieved and stored in: /tmp/test.keytab

Copy link
Member

@pvoborni pvoborni left a comment

Choose a reason for hiding this comment

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

I'd personally combine multiple tests into one because most of them are almost the same. E.g. adding various predefined services can be tested in 1 tests, in a 2 loops - one with DNS, second without. Then removal and add of multiple can be tested and it would save some action in cleanup.

The current way is probably more puristic but requires more execution time and takes more lines of code. I'm not suggesting to rewrite it, just sharing my view. I was never a quality engineer so it might be a wrong view.


ssh_pub = 'div[name="ipasshpubkey"] button[name="add"]'
self.find(ssh_pub, By.CSS_SELECTOR).click()
self.wait_for_request(n=5, d=2)
Copy link
Member

Choose a reason for hiding this comment

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

Why wait for 5 requests with 2-second wait at the end? Clicking on add btw AFAIK doesn't cause any request. Simple self.wait() could be fine here.

self.assert_field_validation_required(parent=service_elem)

@screenshot
def test_add_service_no_DNS_for_host(self):
Copy link
Member

Choose a reason for hiding this comment

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

This is the same tests has the same code as test_add_ldap_service - only different service name which is not important.

@Rezney Rezney force-pushed the web_ui_services branch 4 times, most recently from af5c0a0 to 0858b0f Compare March 18, 2018 18:27
@Rezney
Copy link
Collaborator Author

Rezney commented Mar 19, 2018

@pvoborni All your comments should be now hopefully addressed. Also many tests were squeezed into just two of them.

Also opened freeipa/freeipa-pr-ci#173 to test some other certificate tests.

@pvoborni pvoborni self-assigned this Mar 21, 2018
change get_http_pkey() function to more generic one in
order to get pkey for different services

https://pagure.io/freeipa/issue/7441
Add add_host() support func into test_service to
create temp hosts.

https://pagure.io/freeipa/issue/7441
Add cases for:
"cancel_cert_request", "cancel_hold_cert", "cancel_remove_hold",
"cancel_revoke_cert" and "revoke_cert"

https://pagure.io/freeipa/issue/7441
Add more test cases to test_services. Details in the ticket.

https://pagure.io/freeipa/issue/7441
Add assert_notification() function to check whether
we have a notification of particular type/

https://pagure.io/freeipa/issue/7441
Add assert_field_required() to check whether we
got 'Required field' error message.

https://pagure.io/freeipa/issue/7441
Run shell command on the UI system using "admin"
user's passwd from conf.

https://pagure.io/freeipa/issue/7441
Adjust associations functions to simulate "cancel"
action.

https://pagure.io/freeipa/issue/7441
Add "confirm_btn" to cancel dialog and if "None" return
for confirmation with "Enter" key.

https://pagure.io/freeipa/issue/7441
Move strict "search_btn" element finding to later so we
do not fail when using combobox without search button.
Also switch open_btn.click() before fill_textbox() as it
is used to close the selection.

https://pagure.io/freeipa/issue/7441
@Rezney
Copy link
Collaborator Author

Rezney commented Mar 22, 2018

@pvoborni now all the new tests (test_provision_unprovision_keytab included) are passing.

I guess I should get rid of the "enable WebUI tests" commit as we have still the one ongoing failure, right?

Please let me know if everything is ok.

@pvoborni
Copy link
Member

Yes, it looks OK. Code-wise removing the temp commit and giving ack is good to go. I did not and won't verify if the cases match test plan though.

Run ipa-get/rmkeytab command on UI host in order to test whether
we have the key un/provisioned.

https://pagure.io/freeipa/issue/7441
@Rezney
Copy link
Collaborator Author

Rezney commented Mar 23, 2018

Thx, should be ready for "ack" now. QE POV review will be done afterwards.

@pvoborni pvoborni added the ack Pull Request approved, can be merged label Mar 24, 2018
@tiran
Copy link
Member

tiran commented Mar 24, 2018

master:

  • d8cbd5d ui_tests: change get_http_pkey() function
  • 62a131a ui_tests: add_host() support func in test_service
  • 0f5084b ui_tests: add_service() support func in test_service
  • 735d48d ui_tests: add more test cases to test_certification
  • cd86fd2 ui_tests: add more test cases
  • 01fa541 ui_tests: add assert_notification()
  • 7fb4f75 ui_tests: add assert_field_required()
  • 95de6f0 ui_tests: add funcs to add/remove users public SSH key
  • 18e8c96 ui_tests: add function to run cmd on UI host
  • 5531839 ui_tests: make associations cancelable
  • 16083eb ui_tests: test cancel and delete without button
  • bf1f2d1 ui_tests: select_combobox() fixes
  • 5f87b9c ui_tests: run ipa-get/rmkeytab command on UI host

@tiran tiran added the pushed Pull Request has already been pushed label Mar 24, 2018
@tiran tiran closed this Mar 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
4 participants