Convert Mozmill test 'remote/testSecurity/testSSLStatusAfterRestart.js to Marionette

RESOLVED FIXED in mozilla40

Status

Testing
Firefox UI Tests
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: galgeek, Assigned: galgeek)

Tracking

(Blocks: 1 bug)

unspecified
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

52 bytes, text/x-github-pull-request
chmanchester
: review+
whimboo
: feedback+
chmanchester
: feedback+
Details | Review | Splinter Review
(Assignee)

Description

3 years ago
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
(Assignee)

Updated

3 years ago
Blocks: 1132629
Priority: -- → P1
(Assignee)

Updated

3 years ago
Depends on: 1132940, 1132943
(Assignee)

Comment 1

3 years ago
Created attachment 8581314 [details] [review]
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.
Assignee: nobody → galgeek
Status: NEW → ASSIGNED
Attachment #8581314 - Flags: feedback?(hskupin)
(Assignee)

Updated

3 years ago
Depends on: 1129843
Comment on attachment 8581314 [details] [review]
github_pull_request.txt

Nicely done. Not too much left to do anymore here. Thanks!
Attachment #8581314 - Flags: feedback?(hskupin) → feedback+
(Assignee)

Comment 3

3 years ago
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.
Attachment #8581314 - Flags: feedback?(cmanchester)
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 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.
Attachment #8581314 - Flags: feedback?(cmanchester) → feedback+
(Assignee)

Comment 6

3 years ago
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.
Attachment #8581314 - Flags: review?(cmanchester)
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.
Attachment #8581314 - Flags: review?(cmanchester)
(Assignee)

Comment 8

3 years ago
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.
Attachment #8581314 - Flags: feedback?(hskupin)
Attachment #8581314 - Flags: feedback?(cmanchester)
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!
Attachment #8581314 - Flags: review+
Attachment #8581314 - Flags: feedback?(hskupin)
Attachment #8581314 - Flags: feedback?(cmanchester)
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.
(Assignee)

Comment 11

3 years ago
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.
Attachment #8581314 - Flags: feedback?(hskupin)
(Assignee)

Updated

3 years ago
Attachment #8581314 - Flags: feedback?(hskupin)
https://github.com/mozilla/firefox-ui-tests/commit/d14a060c9e8f0ca11024bfbb1ec9f9bab9e1d227
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Product: Mozilla QA → Testing
You need to log in before you can comment on or make changes to this bug.