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
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
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+
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.
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+
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 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 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 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!
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 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.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
You need to log in before you can comment on or make changes to this bug.