1132651 - Convert Mozmill test 'remote/testSecurity/testGreenLarry.js to Marionette

Status: RESOLVED FIXED

(Testing :: Firefox UI Tests, defect, P1)

Description

[Barbara Miller (:galgeek)]

This bug will cover the necessary work to convert the following test into Marionette.

http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/tests/remote/testSecurity/testGreenLarry.js

Comment 1

[Barbara Miller (:galgeek)]

Attachment: github_pull_request.txt

Many of the variables aren't really necessary, but without them, there are a lot of long lines.

I wonder if something more needs to be added to ensure the identity popup closes.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8574527 [details] [review]
github_pull_request.txt

For comments please see the PR.

Comment 3

[Barbara Miller (:galgeek)]

Comment on attachment 8574527 [details] [review]
github_pull_request.txt

Thank you for your comments, Henrik! I've updated the PR. Travis passes both OS X and Linux.

I've added a method to security.py to provide the domain name from a security cert's "common name" field. I wonder whether getBaseDomain would be helpful here, since the common name field is a host name, and possibly a wildcard host name, like *.example.com, while getBaseDomain expects a URL.

I've left # TODO comments in other places that still need work.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8574527 [details] [review]
github_pull_request.txt

Looks good. Some nits and the dependency we need fixed.

Comment 5

[Barbara Miller (:galgeek)]

Comment on attachment 8574527 [details] [review]
github_pull_request.txt

With the new identity_popup.close(), we need to include the except NoSuchElementException in the try block in tearDown.

The various popup.close() methods are easier to use in tearDown if they check for the popup's presence themselves. Is there a reason not to check for expected.element_present in these methods?

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8574527 [details] [review]
github_pull_request.txt

Looks good. For the next round lets make it a final review. It's close to be landed.

Comment 7

[Barbara Miller (:galgeek)]

Comment on attachment 8574527 [details] [review]
github_pull_request.txt

Thanks for your comments, Henrik!

We still need, for now, the except NoSuchElementException when the test is skipped.

The Travis builds are both green, and this test itself succeeds on AppVeyor, though others fail there...

On the other hand, using --e10s we fail with this:

File "/Users/bara/Dev/ffuit/firefox_ui_tests/remote/security/test_green_larry.py", line 101, in test_green_larry
    self.assertEqual(page_info.deck.selected_panel, page_info.deck.security)
AssertionError: <firefox_puppeteer.ui.pageinfo.deck.GeneralPanel object at 0x103d5e490> != <firefox_puppeteer.ui.pageinfo.deck.SecurityPanel object at 0x103d5e710>

So I'm just requesting feedback for now...

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8574527 [details] [review]
github_pull_request.txt

As mentioned on the PR the e10s failure is mostly because of a bug in Firefox. Try to reproduce it manually, and if not filed can you do that please?

Lets add a workaround for that right now, so that we manually focus the security panel.

Comment 9

[Barbara Miller (:galgeek)]

I was able to reproduce the e10s failure manually, and I've filed bug 1144493.

Comment 10

[Barbara Miller (:galgeek)]

Comment on attachment 8574527 [details] [review]
github_pull_request.txt

I’ve added a workaround to address the e10s bug.

The updated PR passes Travis.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8574527 [details] [review]
github_pull_request.txt

Two items need to be fixed. See my comments on the PR. Feel free to squash all commits when you have those done.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8574527 [details] [review]
github_pull_request.txt

We are close. One more thing to fix and then we can get it landed. Please already squash your commits once you are done with the update. Thanks.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8574527 [details] [review]
github_pull_request.txt

Barbara fixed it and pushed an update which looks pretty fine to me now. Thanks!

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

PR got merged as:
https://github.com/mozilla/firefox-ui-tests/pull/120