Closed Bug 1271911 Opened 4 years ago Closed 4 years ago

TEST-UNEXPECTED-FAIL | test_notifications.py TestNotifications.test_addon_install_failed_notification | AssertionError: AddOnProgressNotification is not an instance of AddOnInstallFailedNotification

Categories

(Testing :: Firefox UI Tests, defect)

49 Branch
All
Windows
defect
Not set
normal

Tracking

(firefox48 fixed, firefox49 fixed)

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: whimboo, Assigned: davehunt)

References

()

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 2 obsolete files)

It's a new test failure as introduced with the implementation bug  	1144873 of notifications to Puppeteer. I will work on a fix together with the improvements on bug 1270953.

 07:23:23     INFO - TEST-UNEXPECTED-FAIL | test_notifications.py TestNotifications.test_add_on_failed_notification | AssertionError: <firefox_puppeteer.ui.browser.notifications.AddOnProgressNotification object at 0x02804310> is not an instance of <class 'firefox_puppeteer.ui.browser.notifications.AddOnInstallFailedNotification'>

 07:23:23     INFO - Traceback (most recent call last):

 07:23:23     INFO -   File "c:\jenkins\workspace\mozilla-central_functional\build\venv\lib\site-packages\marionette\marionette_test.py", line 351, in run

 07:23:23     INFO -     testMethod()

 07:23:23     INFO -   File "c:\jenkins\workspace\mozilla-central_functional\build\tests\firefox-ui\tests\puppeteer\test_notifications.py", line 52, in test_add_on_failed_notification

07:23:23 INFO - AddOnInstallFailedNotification)
It looks like we briefly get an AddOnProgressNotification before the AddOnInstallFailedNotification.
No longer depends on: 1270953
That's not the problem here. With my fix on bug 1270953 I can see that it will be working for nightly builds on mozilla-central and mozilla-aurora with strict signing of extensions enabled. But it does not work for inbound builds!? There extensions are getting installed without the blocked notification.

Dave, do you have an idea what's different for those builds?
Flags: needinfo?(dtownsend)
Attachment #8753366 - Attachment is obsolete: true
Attachment #8753366 - Flags: review?(mjzffr)
Attachment #8753367 - Attachment is obsolete: true
Attachment #8753367 - Flags: review?(mjzffr)
(In reply to Henrik Skupin (:whimboo) from comment #3)
> That's not the problem here. With my fix on bug 1270953 I can see that it
> will be working for nightly builds on mozilla-central and mozilla-aurora
> with strict signing of extensions enabled. But it does not work for inbound
> builds!? There extensions are getting installed without the blocked
> notification.
> 
> Dave, do you have an idea what's different for those builds?

Inbound should be building exactly the same as mozilla-central, there shouldn't be any difference at all.
Flags: needinfo?(dtownsend)
Interesting. So far I tested with a local fragment build. Maybe there is a problem given that a downloaded build from inbound works. I will check what the problem could be.
There was clearly something wrong with the fragment build. I did a fresh pull from inbound and rebuilt, now it is working. So my patch on bug 1270953 will definitely fix the failure on mozilla-central now.
The refactoring didn't fix it completely. It's a Windows only failure now.
Assignee: nobody → hskupin
Blocks: 1265028
Status: NEW → ASSIGNED
No longer depends on: 1270953
OS: All → Windows
Summary: TEST-UNEXPECTED-FAIL | test_notifications.py TestNotifications.test_add_on_failed_notification | AssertionError: AddOnProgressNotification is not an instance of AddOnInstallFailedNotification → TEST-UNEXPECTED-FAIL | test_notifications.py TestNotifications.test_addon_install_failed_notification | AssertionError: AddOnProgressNotification is not an instance of AddOnInstallFailedNotification
I would definitely need help with this. Dave, would be great if you can have a look.
Flags: needinfo?(dave.hunt)
Flags: needinfo?(dave.hunt)
It looks like the test is catching the progress notification popup, which appears briefly when installing a signed add-on. I've created a patch that waits for the expected notification.
Dave, the try results show test failures for linux64 non-e10s. I retriggered once but it keeps showing up. Can you please rebase your patch against latest inbound and upload again? We should trigger a complete new try build. Thanks.
Flags: needinfo?(hskupin)
Assignee: hskupin → dave.hunt
Flags: needinfo?(hskupin) → needinfo?(dave.hunt)
Chris, I have an e10s related question for you which is related to the test changes in that mozreview request. As it looks like Marionette (and in detail our firefox-ui-tests) seem to be the only testsuite which has to set the "browser.tabs.remote.force-enable" preference to True. As what we have seen it at least prevents an additional notification (partially disabled accessibility support) shown on OS X. On Linux I do not see it at all. Is that a good way to go?
Flags: needinfo?(cpeterson)
(In reply to Henrik Skupin (:whimboo) from comment #19)
> Dave, the try results show test failures for linux64 non-e10s. I retriggered
> once but it keeps showing up. Can you please rebase your patch against
> latest inbound and upload again? We should trigger a complete new try build.
> Thanks.

This is likely because I was setting a preference that effectively forced e10s to be enabled. I've moved this in my latest commit to only be set when we're testing against e10s. I've triggered a new try run to see if this passes.
Flags: needinfo?(dave.hunt)
Comment on attachment 8758349 [details]
Bug 1271911 - Wait for the expected notifications in test_notifications.py to avoid transient popups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56612/diff/1-2/
Attachment #8758349 - Attachment description: MozReview Request: Bug 1271911 - Wait for the expected notifications in test_notifications.py to avoid transient popups. r?whimboo → Bug 1271911 - Wait for the expected notifications in test_notifications.py to avoid transient popups.
Attachment #8759186 - Attachment description: MozReview Request: Bug 1271911 - Only force e10s to be enabled if we want it to be enabled. r?whimboo → Bug 1271911 - Only force e10s to be enabled if we want it to be enabled.
Comment on attachment 8759186 [details]
Bug 1271911 - Enable the browser.tabs.remote.force-enable preference when using e10s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57218/diff/1-2/
Comment on attachment 8759186 [details]
Bug 1271911 - Enable the browser.tabs.remote.force-enable preference when using e10s.

https://reviewboard.mozilla.org/r/57218/#review53994

::: testing/marionette/harness/marionette/runner/base.py:440
(Diff revision 2)
>          args.prefs = self._get_preferences(args.prefs_files, args.prefs_args)
>  
>          if args.e10s:
>              args.prefs.update({
>                  'browser.tabs.remote.autostart': True,
> +                'browser.tabs.remote.force-enable': True,

Please request review from Maja or David here. I'm not really a Marionette peer.

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py
(Diff revision 2)
>      def setUp(self, *args, **kwargs):
>          super(BaseFirefoxTestCase, self).setUp(*args, **kwargs)
>  
> -        # setting this pref prevents a notification from being displayed
> -        # when e10s is enabled. see http://mzl.la/1TVNAXh
> -        self.prefs.set_pref('browser.tabs.remote.force-enable', True)

Lets get those lines removed in your first commit, so that we have a clean change for Marionette here.
Attachment #8759186 - Flags: review?(hskupin)
(In reply to Henrik Skupin (:whimboo) from comment #20)
> Chris, I have an e10s related question for you which is related to the test
> changes in that mozreview request. As it looks like Marionette (and in
> detail our firefox-ui-tests) seem to be the only testsuite which has to set
> the "browser.tabs.remote.force-enable" preference to True. As what we have
> seen it at least prevents an additional notification (partially disabled
> accessibility support) shown on OS X. On Linux I do not see it at all. Is
> that a good way to go?

Felipe, do you know the answer to Henrik's question about forcing the "browser.tabs.remote.force-enable" pref in the Marionette tests?
Flags: needinfo?(cpeterson) → needinfo?(felipc)
(In reply to Chris Peterson [:cpeterson] from comment #26)
> Felipe, do you know the answer to Henrik's question about forcing the
> "browser.tabs.remote.force-enable" pref in the Marionette tests?

What I noticed is that Marionette has a11y enabled by default because it relies on some of its features for working with elements. IMO we should get this stopped so I will file a bug for it. But due to that reason and having e10s enabled, we get this extra notification on OS X. If there is another pref we could use to not actually force-enable e10s I would happily use that.
https://reviewboard.mozilla.org/r/56612/#review54282

The test changes look fine to me and try is finally happy! Sadly I won't have the time to get the patch tested on Windows myself. But when you do the commit fixes as I mentioned earlier you will get my r+.
Comment on attachment 8759186 [details]
Bug 1271911 - Enable the browser.tabs.remote.force-enable preference when using e10s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57218/diff/2-3/
Attachment #8759186 - Attachment description: Bug 1271911 - Only force e10s to be enabled if we want it to be enabled. → Bug 1271911 - Enable the browser.tabs.remote.force-enable preference when using e10s.
Attachment #8759186 - Flags: review?(mjzffr)
Attachment #8759186 - Flags: review?(hskupin)
Comment on attachment 8758349 [details]
Bug 1271911 - Wait for the expected notifications in test_notifications.py to avoid transient popups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56612/diff/2-3/
Comment on attachment 8758349 [details]
Bug 1271911 - Wait for the expected notifications in test_notifications.py to avoid transient popups.

https://reviewboard.mozilla.org/r/56612/#review54304

As mentioned earlier the test changes look fine to me. Thanks for your help Dave!
Attachment #8758349 - Flags: review?(hskupin) → review+
Comment on attachment 8759186 [details]
Bug 1271911 - Enable the browser.tabs.remote.force-enable preference when using e10s.

https://reviewboard.mozilla.org/r/57218/#review54306

Having to duplicate all the code now makes me iiiik. It will add a lot of additional maintainance.
Attachment #8759186 - Flags: review?(hskupin)
Attachment #8759186 - Flags: review?(mjzffr) → review+
Comment on attachment 8759186 [details]
Bug 1271911 - Enable the browser.tabs.remote.force-enable preference when using e10s.

https://reviewboard.mozilla.org/r/57218/#review54388
Pushed by mjzffr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e845e5efd230
Enable the browser.tabs.remote.force-enable preference when using e10s. r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/465fe1502bf4
Wait for the expected notifications in test_notifications.py to avoid transient popups. r=whimboo
https://hg.mozilla.org/mozilla-central/rev/e845e5efd230
https://hg.mozilla.org/mozilla-central/rev/465fe1502bf4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Whiteboard: [checkin-needed-aurora]
Maja, seems this is already on aurora. Like

https://hg.mozilla.org/releases/mozilla-aurora/rev/e845e5efd230

so removing flag :)
Whiteboard: [checkin-needed-aurora]
This would need a checkin for beta and not aurora. I think Maja missed to account the last merge.
Whiteboard: [checkin-needed-beta]
Looks like the question asked here about e10s force prefs was understood. If there are any remaining issues about how Marionette and e10s/a11y interacts, then it's probably a good idea to file a new bug for it
Flags: needinfo?(felipc)
You need to log in before you can comment on or make changes to this bug.