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

Fix webui tests #2324

Closed
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@serg-cymbaluk
Contributor

serg-cymbaluk commented Sep 4, 2018

This pull request includes fixes for different issues which I have found while running Web UI tests manually.
There were also two regressions for version 4.6, so both of them have been merged: #2313, #2315

https://pagure.io/freeipa/issue/7686

@abbra

This comment has been minimized.

Contributor

abbra commented Sep 4, 2018

Please add tickets to track these changes.

)
csr = x509.CertificateSigningRequestBuilder()
csr = csr.subject_name(x509.Name([
x509.NameAttribute(NameOID.COMMON_NAME, u'{}'.format(hostname))

This comment has been minimized.

@abbra

abbra Sep 4, 2018

Contributor

I wonder if we should also add a SAN here as CN in the certificates is deprecated and most browsers will remove support for it eventually when SAN is missing.

This comment has been minimized.

@serg-cymbaluk

serg-cymbaluk Sep 6, 2018

Contributor

Something like that?

csr = csr.subject_name(x509.Name([
    x509.NameAttribute(NameOID.COMMON_NAME, hostname)
])).add_extension(x509.SubjectAlternativeName([
    x509.DNSName(hostname)
]))

This comment has been minimized.

@abbra

abbra Sep 6, 2018

Contributor

Yes, this would be it.

This comment has been minimized.

@serg-cymbaluk

serg-cymbaluk Sep 18, 2018

Contributor

Already fixed.

@Rezney

This comment has been minimized.

Collaborator

Rezney commented Sep 4, 2018

@serg-cymbaluk please use a "temp commit" to activate and run WebUI tests upon these changes. Thanks...

@mrizwan93

This comment has been minimized.

Contributor

mrizwan93 commented Sep 4, 2018

@serg-cymbaluk
Thanks for the patch.
You need to make changes at multiple places for replacing hardcoded CSR. I have raised #2214, you can see for places where changes are needed in it.

@pvoborni

Thanks for this PR. Looks as good addition. I did not test, so a temp commit which would run all web UI tests would be useful as already mentioned.

Added some comments - mostly ideas, not hard issues to change.

As @abbra mentioned, a ticket is needed as, I assume, this needs to be merged into 4-6 branch and we track backports in tickets.

Also, as good practice, it is good to mention in commit message "why" the change is needed (motivation), not only what is the change.

try:
assert btn_is_displayed is not True
finally:
self.logout()

This comment has been minimized.

@pvoborni

pvoborni Sep 6, 2018

Member

This will ensure logout only in this one special case. If any assert, after logging in as an unprivileged user, will fail then we are in the same situation - next tests will use the unprivileged user. But still better then the situation now.

Would it make sense to create some sort of e.g. decorator which will attempt to log out in any case after then test ends?

This comment has been minimized.

@serg-cymbaluk

serg-cymbaluk Sep 6, 2018

Contributor

It would be better to add something like driver.delete_all_cookies() to UI_driver.teardown method.
Also It is good idea about decorator, but we can use it for login, not for logout. Because we use init_app() method for this purpose and it is unclear. I'll give you example:

@login.as_admin
def test_crud(self):
    self.basic_crud(ENTITY, self.data)

or

@login.as_('some_prepared_user')
def test_some_permissions(self):
    # some code
return '.'.join(ip)
while True:
new_ip = '10.{}.{}.{}'.format(

This comment has been minimized.

@pvoborni

pvoborni Sep 6, 2018

Member

This change creates different assumptions. But it is probably OK. I don't remember the exact reasons why it used IP from the server's address range. Maybe to add a PTR record into reverse zone automatically. But IPA does not create reverse zones for private IP ranges networks automatically since a certain time, which is usually the case in testing, so it probably does not matter now and the change is probably correct.

@serg-cymbaluk

This comment has been minimized.

Contributor

serg-cymbaluk commented Sep 18, 2018

All Web UI tests have passed on the PR CI.
Also the tests have passed on RHEL (FreeIPA 4.6 + Python 2.7, manual run)

@abbra

This comment has been minimized.

Contributor

abbra commented Sep 18, 2018

@serg-cymbaluk did you check PRCI nightly tests too? There are few web ui tests that fail right now in the nightly PR.

@serg-cymbaluk

This comment has been minimized.

Contributor

serg-cymbaluk commented Sep 18, 2018

@abbra

@serg-cymbaluk did you check PRCI nightly tests too? There are few web ui tests that fail right now in the nightly PR.

There are 4 fails. Two of them have timeout issues (test starts before user's logged in and so on). Other two are new tests, so I'll fix it in a separate branch, because this branch mostly for backporting.

@abbra

abbra approved these changes Sep 18, 2018

@abbra

This comment has been minimized.

Contributor

abbra commented Sep 18, 2018

Please remove the temp commit to give a final run before pushing.

@abbra abbra added ack and removed needs review labels Sep 18, 2018

@serg-cymbaluk

This comment has been minimized.

Contributor

serg-cymbaluk commented Sep 18, 2018

@abbra

Please remove the temp commit to give a final run before pushing.

Done. Waiting for result...

@serg-cymbaluk

This comment has been minimized.

Contributor

serg-cymbaluk commented Sep 19, 2018

master:

  • 2b3fd70 Fix hardcoded CSR in test_webui/test_cert.py
  • 95928f6 Use random IPs and domains in test_webui/test_host.py
  • 1212402 Increase request timeout for WebUI tests
  • d582484 Fix test_realmdomains::test_add_single_labeled_domain (Web UI test)
  • 1f04c48 Use random realmdomains in test_webui/test_realmdomains.py
  • 685cef5 Fix test_user::test_login_without_username (Web UI test)
  • 41258d8 Fix unpermitted user session in test_selfservice (Web UI test)
  • 2b73970 Add SAN extension for CSR generation in test_cert (Web UI tests)
  • b58bc75 Generate CSR for test_host::test_certificates (Web UI test)
  • 93eafae Add cookies clearing for all Web UI tests
  • 970af64 Remove unnecessary session clearing in some Web UI tests
  • 1affdda Increase some timeouts in Web UI tests
  • 46eb9a3 Fix UI_driver.has_class exception. Handle situation when element has no class attribute
  • d020fc4 Change Web UI tests setup flow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment