1228120 - Add tests to ensure SessionStore saves and restores windows and tabs properly after a restart

Status: RESOLVED FIXED

(Testing :: Firefox UI Tests, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 1

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

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/

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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

Comment 3

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

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.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 5

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

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.

Comment 6

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

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

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1228120 - Add tests to ensure SessionStore saves and restores windows and tabs properly after a restart.

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 8

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

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 9

[Mike Conley (:mconley) (:⚙️)]

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/

Comment 10

[Mike Conley (:mconley) (:⚙️)]

New patch is up - clearing ni?.

Comment 11

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

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 12

[Mike Conley (:mconley) (:⚙️)]

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/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/474c03c87e8c