1132701 - Convert Mozmill test 'remote/testSecurity/testSSLStatusAfterRestart.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/testSSLStatusAfterRestart.js

Comment 1

[Barbara Miller (:galgeek)]

Attachment: github_pull_request.txt

I've submitted a WIP PR, with a question about structure noted on the PR.

This test should probably use the restart method from Bug 1129843. I'll record that for now as a block.

Comment 2

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

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

Nicely done. Not too much left to do anymore here. Thanks!

Comment 3

[Barbara Miller (:galgeek)]

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

Chris, thanks (again!) for your time on IRC this afternoon. 

I've tried a couple of different approaches here, but I'm still getting one fewer loaded tab than I should with the code I've pushed. I've tried reordering the test items, and that makes no differences; I get the first two tabs loaded as I expect and a blank third tab.

Note that there really should be four tabs total open after restart, since this code will open a fourth, empty tab before the restart.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

The test failure is due to a difference between mozmill and marionette's restarts: mozmill takes the step of notifying "quit-application-requested" when it does a restart, while marionette does not. Sessionstore takes a snapshot of open tabs in "quit-application-requested", so if I add:

+        self.marionette.execute_script("""
+          Components.utils.import("resource://gre/modules/Services.jsm");
+          let cancelQuit = Components.classes["@mozilla.org/supports-PRBool;1"]
+                                     .createInstance(Components.interfaces.nsISupportsPRBool);
+          Services.obs.notifyObservers(cancelQuit, "quit-application-requested", null);
+        """)

directly before the "self.restart()" in the test it passes. This is something that can probably be added to the firefox-ui-tests restart() method, but for finest granularity tests can make a point to notify this observer or not.

Comment 5

[Chris Manchester (limited bugmail, email directly)]

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

Looks good so far. Glad to have another look once we're satisfied this is passing and looking good.

Comment 6

[Barbara Miller (:galgeek)]

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

Thanks for your feedback, Chris!

I've added the code you suggested and filed a bug to add the capability to the restart() method.

Comment 7

[Chris Manchester (limited bugmail, email directly)]

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

I left a few comments that should be addressed, mostly small changes. I need to look at the mozmill test to see about the question about what we're doing doing differently from the mozmill for certs with wildcards in them, I saw a question about that on github without an answer. I'll try to track that down before reviewing again.

Comment 8

[Barbara Miller (:galgeek)]

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

Thanks for your comments, Chris!

I've updated the PR and added a couple of line notes on github.

I'm just requesting feedback now, wondering if we can land this before the execute.script code finds its final home.

Comment 9

[Chris Manchester (limited bugmail, email directly)]

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

This looks good. I think putting the observer notification code before the restart in a shared place in firefox-ui-tests would make sense for now, but no need to block on that. Thanks for your work on this!

Comment 10

[Chris Manchester (limited bugmail, email directly)]

The one thing left to check here is that the modification to the restart method doesn't have unintended consequences for the update tests. Once we confirm that this test can land.

Comment 11

[Barbara Miller (:galgeek)]

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

Thanks for your comments, Chris!

My latest commit moves the quit-application-requested code to the harness, until it can be implemented in marionette itself. This passes current Travis tests.

In testing this commit with update tests, we discovered and Chris reported Bug 1149818 and submitted a fix, awaiting review. If we want to include my latest commit, we should wait for this bug to land.

Comment 12

[Chris Manchester (limited bugmail, email directly)]

https://github.com/mozilla/firefox-ui-tests/commit/d14a060c9e8f0ca11024bfbb1ec9f9bab9e1d227