Closed Bug 1291844 Opened 8 years ago Closed 7 years ago

Disable test_restore_windows_after_restart.py on Windows due to perma failures

Categories

(Testing :: Firefox UI Tests, defect)

Product:
Testing
Component:
Firefox UI Tests
Version:
Version 3
Platform:
All
Windows
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox50 disabled, firefox51 disabled, firefox-esr52 disabled, firefox53 disabled, firefox54 disabled, firefox55 disabled, firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox50 --- disabled
firefox51 --- disabled
firefox-esr52 --- disabled
firefox53 --- disabled
firefox54 --- disabled
firefox55 --- disabled
firefox56 --- fixed

People

(Reporter: whimboo, Assigned: ewright)

References

(Depends on 2 open bugs)

Details

Attachments

(2 files)

Bug 1291844 - Disable test_restore_windows_after_restart.py on Windows due to perma failures.
58 bytes, text/x-review-board-request
mconley
: review+
whimboo
: checked-in+
Details
Bug 1291844 - Add negative test and quit test to restore windows test file.
59 bytes, text/x-review-board-request
whimboo
: review+
Details 
The test as implemented on bug 1228120 perma fails on Windows right now. Mike won't be able to get to it today and maybe also not tomorrow. I would suggest we get this test disabled on mozilla-central and mozilla-aurora.
Attachment #8777516 - Flags: review?(mconley) → review+
Comment on attachment 8777516 [details]
Bug 1291844 - Disable test_restore_windows_after_restart.py on Windows due to perma failures.

https://reviewboard.mozilla.org/r/69058/#review66138
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/db8a3cda72f9
Disable test_restore_windows_after_restart.py on Windows due to perma failures. r=mconley
https://hg.mozilla.org/mozilla-central/rev/db8a3cda72f9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
We also need this test-only change on mozilla-aurora. Asking for a backport.
Whiteboard: [checkin-needed-aurora]
Depends on: 1291838
Depends on: 1291840
This bug should not have been closed given that the test got disabled on Windows for e10s. 

A lot of code has been changed meanwhile and it might make sense to try to re-enable the test. Lets wait for bug 1371404 being on mozilla-central, so that we can trigger try builds for fxui tests on Windows.
Status: RESOLVED → REOPENED
status-firefox50: fixed → disabled
status-firefox51: fixed → disabled
status-firefox53: --- → disabled
status-firefox54: --- → disabled
status-firefox55: --- → disabled
status-firefox-esr52: --- → disabled
Depends on: 1371404
Resolution: FIXED → ---
I just noticed yesterday that quit() and restart() are kinda broken and in some cases throws away the profile. Given that this can conflict with sessionrestore I would try to re-enable the tests once bug 1373635 landed.
Depends on: 1373635
:whimboo, I changed this test to `restart` in a similar way to bug 1219725. As well, I added a test that checks the opposite case (for when the auto-restore tabs pref is NOT enforced). Pushed it to try, and windows and e10s both seem to pass. Would you like me to attach a reviewboard patch?
Flags: needinfo?(hskupin)
(In reply to Erica from comment #10)
> :whimboo, I changed this test to `restart` in a similar way to bug 1219725.
> As well, I added a test that checks the opposite case (for when the
> auto-restore tabs pref is NOT enforced). Pushed it to try, and windows and
> e10s both seem to pass. Would you like me to attach a reviewboard patch?

This is great to hear! You were even faster than me here, and which frees up some time for me. So yes, I'm totally interested to get a patch. Thanks a lot!
Flags: needinfo?(hskupin)
Assignee: hskupin → ewright
Status: REOPENED → ASSIGNED
Hardware: Unspecified → All
Target Milestone: mozilla51 → ---
Comment on attachment 8883591 [details]
Bug 1291844 - Add negative test and quit test to restore windows test file.

https://reviewboard.mozilla.org/r/154518/#review160178

Thank you for taking care of this test, Erica! I appreciate it. As following some notes to further improvements, which might be good to have too while you already update the test.

::: testing/firefox-ui/tests/functional/sessionstore/manifest.ini
(Diff revision 1)
>  [DEFAULT]
>  tags = local
>  
>  [test_tabbar_session_restore_button.py]
>  [test_restore_windows_after_restart.py]
> -skip-if = (os == "win" || e10s) # Bug 1291844 and Bug 1228446

I think we have to rename the test filename, so it also includes `quit`.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart.py:66
(Diff revision 1)
>          restored, and that the private ones have not.
>          """
>          self.open_windows(self.test_windows)
>          self.open_windows(self.private_windows, is_private=True)
>  
> -        self.restart()
> +        self.marionette.quit(in_app=True)

Lets make sure we also have a test for the `restart()` call and `browser.startup.page` set to `1`. That should also restore the session.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart.py:88
(Diff revision 1)
>              opened_windows.add(urls)
>  
>          self.assertEqual(opened_windows, self.test_windows)
>  
> +    def test_without_restore(self):
> +        self.marionette.enforce_gecko_prefs({'browser.startup.page': 1})

This is the default setting in Marionette. Maybe just assert that the value is not `3` here, so we can save us an additional, unnecessary restart. An adjustment of setUp might have to be done.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart.py:97
(Diff revision 1)
> +
> +        self.marionette.quit(in_app=True)
> +        self.marionette.start_session()
> +        self.marionette.set_context('chrome')
> +
> +        windows = self.puppeteer.windows.all

No need for this extra variable. Just use it directly in your assertion.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart.py:100
(Diff revision 1)
> +        self.marionette.set_context('chrome')
> +
> +        windows = self.puppeteer.windows.all
> +
> +        # The 3 windows from setUp did not re-open.
> +        self.assertEqual(len(windows), 1)

Better use a message for the assertion as the comment. So `Windows from last session shouldn't have been reopened`.

Also I would add an assertion that the tabs haven't been restored.
Attachment #8883591 - Flags: review?(hskupin) → review-
Comment on attachment 8883591 [details]
Bug 1291844 - Add negative test and quit test to restore windows test file.

https://reviewboard.mozilla.org/r/154518/#review160178

> This is the default setting in Marionette. Maybe just assert that the value is not `3` here, so we can save us an additional, unnecessary restart. An adjustment of setUp might have to be done.

I initially attemtped to change setUp and remove the enforcing of 3 from there. However, enforcing 3 from within `test_with_variety` brings back our favorite error of "TypeError: win.outerWindowID is undefined" from `tab.location` when it loops through the tabs to compare them.

I've learned now that using `enforce_gecko_prefs` within the testing method will trigger the "win.outerWindowID is undefined" error, while setting it in the setUp method will not. I'm going to push the code which is currently broken for your reference. A proposed work-around would be to have two classes, one inherit from the other and set the different pref in each `setUp` method. Do you agree with this?

I left the explicit setting of `browser.startup.page': 1` just to be clear what we are testing as well as if the browser default changes.
Flags: needinfo?(hskupin)
(In reply to Erica from comment #15)
> I've learned now that using `enforce_gecko_prefs` within the testing method
> will trigger the "win.outerWindowID is undefined" error, while setting it in
> the setUp method will not. I'm going to push the code which is currently

The problem here is that the second call to `enforce_gecko_prefs` is resetting the formerly set preferences, and as such 'browser.sessionstore.restore_on_demand' is 'True' again. As such only the currently selected tab is getting restored, but not any other background tab.

So I would suggest that we define the set of necessary base prefs in setUp, and overwrite the setting for 'browser.startup.page' in each test, which all of them call `enforce_gecko_prefs` themselves. So we don't call it anytime longer in setUp.

I will file a bug so it won't cause a JS error in Firefox Puppeteer in case of such a situation.
Depends on: 1260502
Flags: needinfo?(hskupin)
Depends on: 1373191
Depends on: 1351940
Attachment #8777516 - Flags: checked-in+
Comment on attachment 8883591 [details]
Bug 1291844 - Add negative test and quit test to restore windows test file.

https://reviewboard.mozilla.org/r/154518/#review160178

> I initially attemtped to change setUp and remove the enforcing of 3 from there. However, enforcing 3 from within `test_with_variety` brings back our favorite error of "TypeError: win.outerWindowID is undefined" from `tab.location` when it loops through the tabs to compare them.
> 
> I've learned now that using `enforce_gecko_prefs` within the testing method will trigger the "win.outerWindowID is undefined" error, while setting it in the setUp method will not. I'm going to push the code which is currently broken for your reference. A proposed work-around would be to have two classes, one inherit from the other and set the different pref in each `setUp` method. Do you agree with this?
> 
> I left the explicit setting of `browser.startup.page': 1` just to be clear what we are testing as well as if the browser default changes.

The workaround would also be fine for me, so that we can leave the call to `enforce_gecko_prefs` in `setUp`.
Comment on attachment 8883591 [details]
Bug 1291844 - Add negative test and quit test to restore windows test file.

https://reviewboard.mozilla.org/r/154518/#review162028
Attachment #8883591 - Flags: review?(hskupin)
Comment on attachment 8883591 [details]
Bug 1291844 - Add negative test and quit test to restore windows test file.

https://reviewboard.mozilla.org/r/154518/#review162876

I like how this tests are getting improved! We will get a way better coverage for all the differnt scenarios. I put a couple of comments in this review, so please don't be afraid. It's just my experience from failure reports in the last years. Everything which helps to make it better understandable should be done.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py:1
(Diff revision 3)
> +# This Source Code Form is subject to the terms of the Mozilla Public

Did you rename the file or re-create it? If you did the latter please rename it so we can easier keep the history.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py:11
(Diff revision 3)
> +from marionette_harness import MarionetteTestCase
> +
> +
> +class TestBaseRestoreWindows(PuppeteerMixin, MarionetteTestCase):
> +
> +    def setUp(self, prefValue=1):

No camelcase in Python code please. Maybe use a more descriptive name here too like `startup_page`.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py:125
(Diff revision 3)
> +                        with self.marionette.using_context('chrome'):
> +                            win.tabbar.open_tab()
> +                    self.marionette.navigate(url)
> +
> +
> +class TestRestoreWindowsPrefThree(TestBaseRestoreWindows):

Please name it `TestSessionStoreEnabled` for a better understanding when seeing test failures.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py:127
(Diff revision 3)
> +                    self.marionette.navigate(url)
> +
> +
> +class TestRestoreWindowsPrefThree(TestBaseRestoreWindows):
> +    def setUp(self):
> +        super(TestRestoreWindowsPrefThree, self).setUp(3)

Use it like a keyword, so it's clear what it means, like `setUp(startup_page=3)`.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py:138
(Diff revision 3)
> +        restored, and that the private ones have not.
> +        """
> +
> +        self.open_windows(self.test_windows)
> +        self.open_windows(self.private_windows, is_private=True)
> +

Can you add checks that the right number of windows and tabs are open? If something failed with that step, we would hide real issues.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py:156
(Diff revision 3)
> +        opened_windows = set()
> +        for win in windows:
> +            urls = tuple()
> +            for tab in win.tabbar.tabs:
> +                urls = urls + tuple([tab.location])
> +            opened_windows.add(urls)

We have that code twice. Lets move it to the base class and make it its own method.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py:160
(Diff revision 3)
> +                urls = urls + tuple([tab.location])
> +            opened_windows.add(urls)
> +
> +        # Windows and tabs from last session have been restored,
> +        # excepting the private window.
> +        self.assertEqual(opened_windows, self.test_windows)

Please move the comment into the assertion as message. That's more helpful as seeing `1 != 3` on Treeherder.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py:163
(Diff revision 3)
> +        # Windows and tabs from last session have been restored,
> +        # excepting the private window.
> +        self.assertEqual(opened_windows, self.test_windows)
> +
> +
> +class TestRestoreWindowsPrefOne(TestBaseRestoreWindows):

I would give it a better name like `TestSessionStoreDisabled`. It would be the same for the pref value `0`

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py:165
(Diff revision 3)
> +        self.assertEqual(opened_windows, self.test_windows)
> +
> +
> +class TestRestoreWindowsPrefOne(TestBaseRestoreWindows):
> +    def setUp(self):
> +        super(TestRestoreWindowsPrefOne, self).setUp()

It's a no-op. You can remove setUp here.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py:165
(Diff revision 3)
> +        self.assertEqual(opened_windows, self.test_windows)
> +
> +
> +class TestRestoreWindowsPrefOne(TestBaseRestoreWindows):
> +    def setUp(self):
> +        super(TestRestoreWindowsPrefOne, self).setUp()

This is a no-op. You can remove `setUp` here.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py:167
(Diff revision 3)
> +
> +class TestRestoreWindowsPrefOne(TestBaseRestoreWindows):
> +    def setUp(self):
> +        super(TestRestoreWindowsPrefOne, self).setUp()
> +
> +    def test_without_restore(self):

`test_no_restore_with_quit`.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py:170
(Diff revision 3)
> +        super(TestRestoreWindowsPrefOne, self).setUp()
> +
> +    def test_without_restore(self):
> +        self.open_windows(self.test_windows)
> +        self.open_windows(self.private_windows, is_private=True)
> +

Please check that the correct number of windows and tabs have been opened before the call to `quit`.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py:178
(Diff revision 3)
> +        self.marionette.set_context('chrome')
> +
> +        # Windows from last session shouldn't have been reopened.
> +        self.assertEqual(len(self.puppeteer.windows.all), 1)
> +        # Tabs from last session shouldn't have been reopened.
> +        self.assertEqual(len(self.puppeteer.windows.current.tabbar.tabs), 1)

Please move both comments as message into the assertions. This would give way more information if it fails in automation. By doing that make sure that you negate the wording compared what is expected.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py:183
(Diff revision 3)
> +        self.assertEqual(len(self.puppeteer.windows.current.tabbar.tabs), 1)
> +
> +    def test_restore_with_restart(self):
> +        self.open_windows(self.test_windows)
> +        self.open_windows(self.private_windows, is_private=True)
> +

Same as above. Please check the correct number of window and tabs.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py:197
(Diff revision 3)
> +                urls = urls + tuple([tab.location])
> +            opened_windows.add(urls)
> +
> +        # Windows and tabs from last session have been restored,
> +        # excepting the private window.
> +        self.assertEqual(opened_windows, self.test_windows)

Move the comment as message into the assertion.
Attachment #8883591 - Flags: review?(hskupin) → review-
Comment on attachment 8883591 [details]
Bug 1291844 - Add negative test and quit test to restore windows test file.

https://reviewboard.mozilla.org/r/154518/#review162876

> Please move the comment into the assertion as message. That's more helpful as seeing `1 != 3` on Treeherder.

I have not been able to find a way to add comments or messages to assertions. can you show ma an example?
Comment on attachment 8883591 [details]
Bug 1291844 - Add negative test and quit test to restore windows test file.

https://reviewboard.mozilla.org/r/154518/#review162876

> I have not been able to find a way to add comments or messages to assertions. can you show ma an example?

Just do:

```
self.assertEqual(opened_windows, self.test_windows,
                 msg="My own message to display with reversed meaning to the assertion check")
```

See also https://docs.python.org/2/library/unittest.html#unittest.TestCase.assertEqual
Comment on attachment 8883591 [details]
Bug 1291844 - Add negative test and quit test to restore windows test file.

https://reviewboard.mozilla.org/r/154518/#review162876

> Can you add checks that the right number of windows and tabs are open? If something failed with that step, we would hide real issues.

done, I was thinking of putting it in the setUp method since all three tests use it, but then read that assertions in setUp are not good practice, do you have an opinion?
Comment on attachment 8883591 [details]
Bug 1291844 - Add negative test and quit test to restore windows test file.

https://reviewboard.mozilla.org/r/154518/#review162876

> Did you rename the file or re-create it? If you did the latter please rename it so we can easier keep the history.

Thanks, that is way better now!
Comment on attachment 8883591 [details]
Bug 1291844 - Add negative test and quit test to restore windows test file.

https://reviewboard.mozilla.org/r/154518/#review163444

r=me with the remaining requested changes and a green try build, which I haven't seen yet. Thanks for working on this Erica!

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py:159
(Diff revision 5)
> +        some number of tabs in them. Once the tabs have loaded, restarts
> +        the browser, and then ensures that the standard tabs have been
> +        restored, and that the private ones have not.
> +        """
> +        self.assertEqual(self.convert_open_windows_to_set(), self.all_windows,
> +                         msg='The test starts with the correct amount of windows and tabs.')

Please use the negative wording here as I pointed out in my last review. Like 'Not all requested windows have been opened'.

Please check all the other cases too.
Attachment #8883591 - Flags: review?(hskupin) → review+
Comment on attachment 8883591 [details]
Bug 1291844 - Add negative test and quit test to restore windows test file.

https://reviewboard.mozilla.org/r/154518/#review163670

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py:179
(Diff revisions 5 - 6)
>          self.marionette.quit(in_app=True)
>          self.marionette.start_session()
>          self.marionette.set_context('chrome')
>  
>          self.assertEqual(len(self.puppeteer.windows.all), 1,
>                           msg='Windows from last session shouldn`t have been restored.')

The message of this type of assertion also needs an update. Check others too.
Comment on attachment 8883591 [details]
Bug 1291844 - Add negative test and quit test to restore windows test file.

https://reviewboard.mozilla.org/r/154518/#review163670

> The message of this type of assertion also needs an update. Check others too.

But, this is already a negative. If the test fails, it explains that the windows "should not" have been restored.
The others as well, have already been changed to explain what went wrong in the negative.
Comment on attachment 8883591 [details]
Bug 1291844 - Add negative test and quit test to restore windows test file.

https://reviewboard.mozilla.org/r/154518/#review163670

> But, this is already a negative. If the test fails, it explains that the windows "should not" have been restored.
> The others as well, have already been changed to explain what went wrong in the negative.

Ups. Sorry, you are right. Please disregard. And it looks like that we can land your patch now!
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/33dd57cd0263
Add negative test and quit test to restore windows test file. r=whimboo
Blocks: 1382154
https://hg.mozilla.org/mozilla-central/rev/33dd57cd0263
Status: ASSIGNED → RESOLVED
Closed: 8 years ago → 7 years ago
status-firefox56: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1382453
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: