I'm changing the behaviour of application shutdown in bug 1177310. Normally I'd write a mochitest-browser test to exercise SessionStore changes, but testing this correctly means actually restarting the browser, which mochitest cannot do. :whimboo suggested I write a Firefox UI test instead.

Firefox UI tests are based on Marionette and use a puppeteer library for easy access to ui elements. As talked with Mike on IRC that is what he wants to use. How to get started: https://github.com/mozilla/firefox-ui-tests/blob/mozilla-central/CONTRIBUTING.md Some basic docs for puppeteer we have here: http://firefox-puppeteer.readthedocs.org/en/latest/ui/browser/window.html Best place to get this test added would be at: https://github.com/mozilla/firefox-ui-tests/tree/mozilla-central/firefox_ui_tests/functional/sessionstore/

WIP pull request here: https://github.com/mozilla/firefox-ui-tests/pull/292

Bug 1141483 is not a blocker directly for this test. Given that most of the test will have to work in chrome scope (right?) you can use the `self.restart()` method from the FirefoxTestcase class, which sets the chrome scope transparently.

Oh, I see - so what is still preventing me from inspecting the tabs after restart in the e10s case? Which bug do I lean on to make that work?

For now lets get this test working for non e10s mode. As we have seen this should work. And once the already marked dependency has been fixed (it still needs further investigation) we could turn on this test for running in e10s mode.

Oh, I think that bug 1228446 should still be a blocker for the e10s case.

Now that the Firefox UI tests are in the tree, this is possible and less of a pain. Unfortunately, due to bug 1228446, this test is disabled for e10s. Review commit: https://reviewboard.mozilla.org/r/62944/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/62944/

Comment on attachment 8768967 [details] Bug 1228120 - Add tests to ensure SessionStore saves and restores windows and tabs properly after a restart. https://reviewboard.mozilla.org/r/62944/#review62576 Nice test for the sessionstore feature! It's indeed something we missed so far. Mike, I have added a couple of comments below. Most of those might not be that obvious for someone who is getting started with our tests. So if you think that we can make improvements please let me know. ::: testing/firefox-ui/tests/functional/sessionstore/manifest.ini:5 (Diff revision 1) > +[DEFAULT] > +tags = local > + > +[test_restore_windows_after_restart.py] > +skip-if = e10s # Bug 1228446 You will also have to include this manifest from `functional/manifest.ini`. Otherwise the test won't be run at all. ::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart.py:20 (Diff revision 1) > + self.test_windows = [ > + # Window 1 > + [self.marionette.absolute_url('layout/mozilla.html')], > + > + # Window 2 > + [self.marionette.absolute_url('layout/mozilla.html'), Maybe we use a different url here to not have duplicates with the first window? In this case tab 1 would be the same in both windows. ::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart.py:44 (Diff revision 1) > + # the user to click on the background tabs > + self.prefs.set_pref('browser.sessionstore.restore_on_demand', False) > + # Avoid race conditions by having the content process never > + # send us session updates unless the parent has explicitly asked > + # for them via the TabStateFlusher. > + self.prefs.set_pref('browser.sessionstore.debug.no_auto_updates', True) You will have to enforce those preferences. Otherwise they won't survive the restart, and are getting reset by Marionette. See http://marionette-client.readthedocs.io/en/latest/reference.html?highlight=enforce#marionette_driver.marionette.Marionette.enforce_gecko_prefs ::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart.py:47 (Diff revision 1) > + # send us session updates unless the parent has explicitly asked > + # for them via the TabStateFlusher. > + self.prefs.set_pref('browser.sessionstore.debug.no_auto_updates', True) > + > + def tearDown(self): > + FirefoxTestCase.tearDown(self) Best would be to instruct Marionette to create a new profile and restart again. That way we won't leave any traces behind. Otherwise you will have to at least reset the enforced preferences, and close any additionally opened browser window. ::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart.py:60 (Diff revision 1) > + self.open_windows(self.test_windows) > + self.open_windows(self.private_windows, is_private=True) > + > + self.restart() > + > + with self.marionette.using_context('chrome'): This is not necessary. The `restart()` method enables chrome scope automatically. ::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart.py:69 (Diff revision 1) > + self.assertEqual(len(windows), len(self.test_windows)) > + > + for win_index, win in enumerate(windows): > + self.windows.switch_to(win.handle) > + > + expected_tab_urls = self.test_windows[win_index] This can cause a race because the order of chrome windows is just random in Marionette. So the index of the current window does not necessarly match the index of the original window list! If that passes for you, you were lucky! ::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart.py:74 (Diff revision 1) > + expected_tab_urls = self.test_windows[win_index] > + for tab_index, tab in enumerate(win.tabbar.tabs): > + expected_tab_url = expected_tab_urls[tab_index] > + self.assertEqual(expected_tab_url, tab.location) > + > + for win in windows[1:]: It's better to call `self.windows.close_all([self.browser])` in tearDown. ::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart.py:78 (Diff revision 1) > + > + for win in windows[1:]: > + win.close() > + > + > + def open_windows(self, urlLists, is_private=False): nit: maybe s/urlList/windowList/ ? ::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart.py:79 (Diff revision 1) > + for win in windows[1:]: > + win.close() > + > + > + def open_windows(self, urlLists, is_private=False): > + if (is_private): Maybe add a comment here that we special handle already open windows. Maybe you can use `enumerate()` here to handle the first index specifically in the for loop below. With that you can remove the call to `open_tabs()` for index 0, which actually would correctly be handled in `open_tabs()` itself, right? ::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart.py:96 (Diff revision 1) > + > + > + def open_tabs(self, win, urls): > + # Send the initial tab to the first URL... > + with self.marionette.using_context('content'): > + self.marionette.navigate(urls[0]) In this case urls won't be empty, but if it is we would run into an index error here. Why not doing it similar to what I mentioned above for the window loop?

Comment on attachment 8768967 [details] Bug 1228120 - Add tests to ensure SessionStore saves and restores windows and tabs properly after a restart. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62944/diff/1-2/

New patch is up - clearing ni?.

Comment on attachment 8768967 [details] Bug 1228120 - Add tests to ensure SessionStore saves and restores windows and tabs properly after a restart. https://reviewboard.mozilla.org/r/62944/#review64990 Great update Mike! There is one thing I would like to see fixed. Otherwise the test looks great. Not sure if you have the possibility to also check if your tests passes on Windows and OS X locally. As of now we only run it on Linux via Taskcluster in Try. So in case of failures on other platforms we would see it first for Nightly builds when tests are run in mozmill-ci. ::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart.py:50 (Diff revisions 1 - 2) > - # for them via the TabStateFlusher. > + # for them via the TabStateFlusher. > - self.prefs.set_pref('browser.sessionstore.debug.no_auto_updates', True) > + 'browser.sessionstore.debug.no_auto_updates': True, > + }) > > def tearDown(self): > + self.windows.close_all([self.browser]) I would say we skip this line given that we reset the profile with the call to `restart()`. So no additional windows should be open. Also in all other tests we use `try: finally` to make sure we definitely call `FirefoxTestCase.tearDown(self)` in case of any possible exception. Maybe it would be good doing the same here too. ::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart.py:83 (Diff revisions 1 - 2) > + urls = urls + tuple([tab.location]) > + opened_windows.add(urls) > + > + self.assertEqual(opened_windows, self.test_windows) > + > + def open_windows(self, window_sets, is_private=False): Nice helper method! Maybe we can consider to move it to the base testcase class later when we might want to use it in another test. ::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart.py:147 (Diff revisions 1 - 2) > + if isinstance(urls, str): > + self.marionette.navigate(urls) > + else: > + for index, url in enumerate(urls): > + if index > 0: > + with self.marionette.using_context('chrome'): Way lesser context switching. Nice.

Comment on attachment 8768967 [details] Bug 1228120 - Add tests to ensure SessionStore saves and restores windows and tabs properly after a restart. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62944/diff/2-3/

Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/474c03c87e8c Add tests to ensure SessionStore saves and restores windows and tabs properly after a restart. r=whimboo