Implement a UI class for notification popups

RESOLVED FIXED in Firefox 48

Status

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: chmanchester, Assigned: davehunt)

Tracking

Version 3
mozilla48
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

This is like bug 1139544, but for doorhanger notifications.
Previous code for our Mozmill tests can be found in that file:
http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/lib/toolbars.js

Keep in mind that we haven't had a complete class before but only helper methods on the location bar class. So it would be a new implementation, not a trivial transplant.
Product: Mozilla QA → Testing
Assignee: nobody → dave.hunt
Comment on attachment 8739473 [details]
MozReview Request: Bug 1144873 - Implement a UI class for notification popups r?sydpolk, maja_zf

https://reviewboard.mozilla.org/r/45279/#review41785
Attachment #8739473 - Flags: review?(mjzffr) → review+
Thanks Maja, I also wanted to share a working example of this in use. Here's a gist for pytest fixtures and simple test using Puppeteer and notifications to install an add-on from AMO: https://gist.github.com/davehunt/e636639374f280316ac0ca560c522d26

I haven't included any tests in my patch, because I wasn't sure how to invoke notifications from Firefox without involving external resources. If you have any suggestions, I'd feel much happier including tests.
Flags: needinfo?(mjzffr)

Updated

3 years ago
Attachment #8739473 - Flags: review?(spolk) → review+

Comment 5

3 years ago
Comment on attachment 8739473 [details]
MozReview Request: Bug 1144873 - Implement a UI class for notification popups r?sydpolk, maja_zf

https://reviewboard.mozilla.org/r/45279/#review42081

Is there code that actually calls this somewhere? Is there a test? Probably not. It looks fine, but I am curious as to how this will be used.
Thanks for the gist, Dave -- very helpful. I was wondering the same about testing. I noticed that test_addons.py [1] uses mn-restartless-*.xpi for testing. Maybe you could use it, too, to test the notification?

[1] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/tests/unit/test_addons.py
Flags: needinfo?(mjzffr)
I've added tests that use an unsigned add-on to trigger a popup. If we had a signed add-on we could test a few more popups, but I'm comfortable with these basic tests. I'm not sure where these tests are run though, and it seems odd that they're not in testing/puppeteer/firefox/tests. When I run the full suite of puppeteer tests locally, I get some failures, too.

Comment 9

3 years ago
Comment on attachment 8741274 [details]
MozReview Request: Add tests for notifications and set the BaseNotification as the default in wait_for_notification r?sydpolk, maja_zf

https://reviewboard.mozilla.org/r/46349/#review43021

Thanks for adding the tests.
Attachment #8741274 - Flags: review?(spolk) → review+
Comment on attachment 8741274 [details]
MozReview Request: Add tests for notifications and set the BaseNotification as the default in wait_for_notification r?sydpolk, maja_zf

https://reviewboard.mozilla.org/r/46349/#review43165

Yay for tests! Thanks.

When I ran these locally they only passed with e10s disabled. Is that expected?

::: testing/firefox-ui/tests/puppeteer/test_notifications.py:50
(Diff revision 1)
> +    def trigger_add_on_notification(self, add_on):
> +        path = os.path.join(here, add_on)
> +        with self.marionette.using_context('content'):
> +            self.marionette.execute_script(
> +                'document.location = "file://{0}";'.format(path))
> +        self.browser.wait_for_notification()

Could you specify the notification class here? The failure message I see now is kind of vague: "BaseNotification not present".
Attachment #8741274 - Flags: review?(mjzffr)
https://reviewboard.mozilla.org/r/46349/#review43165

It was not expected, but I've found a way to get them passing in e10s.

> Could you specify the notification class here? The failure message I see now is kind of vague: "BaseNotification not present".

I've improved the message here, but I can't specify the notification class because it will be different when we have a signed add-on.
Comment on attachment 8739473 [details]
MozReview Request: Bug 1144873 - Implement a UI class for notification popups r?sydpolk, maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45279/diff/1-2/
Attachment #8741274 - Attachment is obsolete: true
Attachment #8739473 - Flags: review+ → review?(mjzffr)
Comment on attachment 8739473 [details]
MozReview Request: Bug 1144873 - Implement a UI class for notification popups r?sydpolk, maja_zf

https://reviewboard.mozilla.org/r/45279/#review43417
Attachment #8739473 - Flags: review?(mjzffr) → review+
Looks good, works locally, land as you see fit. The firefox-ui jobs are failing for lots of other reasons, but you should still be able to see your new tests passing somewhere amid the failures. :)

Comment 16

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8871f41bc394
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
https://reviewboard.mozilla.org/r/45279/#review47709

Hey Dave! Thanks a ton for working on the implementation for doorhanger notifications! It's great to see that those have been landed now. 

Even this bug is fixed I checked the code and made some comments. There is nothing we have to work on this bug, but given the comment Bugzilla will create, I will file new bug for enhancements to this patch.

::: testing/firefox-ui/tests/puppeteer/test_notifications.py:75
(Diff revision 2)
> +        with self.assertRaisesRegexp(TimeoutException, message):
> +            self.browser.wait_for_notification(None)
> +
> +    def trigger_add_on_notification(self, add_on):
> +        with self.marionette.using_context('content'):
> +            self.marionette.navigate('file://{0}'.format(here))

For resources we have a separate folder under http://mxr.mozilla.org/mozilla-central/source/testing/firefox-ui/resources/. It should have been the place where the XPI is stored. We can fix that in a follow-up bug. Also we should not install via file:// but localhost://

::: testing/puppeteer/firefox/firefox_puppeteer/ui/browser/notifications.py:5
(Diff revision 2)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +# You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +from abc import ABCMeta

We haven't used the meta classes anywhere else yet. Introducing them somewhere in the middle of the class hierachy is a bit irritating. But lets get a new bug filed to get the basic classes updated.

::: testing/puppeteer/firefox/firefox_puppeteer/ui/browser/notifications.py:32
(Diff revision 2)
> +
> +        :returns: The notification origin.
> +        """
> +        return self.element.get_attribute('origin')
> +
> +    def close(self):

We need a `force` option here, so we can ensure at least in teardown that the notification gets definitely closed.

::: testing/puppeteer/firefox/firefox_puppeteer/ui/browser/notifications.py:35
(Diff revision 2)
> +        return self.element.get_attribute('origin')
> +
> +    def close(self):
> +        """Close the notification."""
> +        self.element.find_element(
> +            By.ANON_ATTRIBUTE, {'anonid': 'closebutton'}).click()

Usually we have a separate property for elements, so we can act on them directly and not via helper methods.

::: testing/puppeteer/firefox/firefox_puppeteer/ui/browser/notifications.py:45
(Diff revision 2)
> +
> +    def allow(self):
> +        """Allow the add-on to be installed."""
> +        self.element.find_element(
> +            By.ANON_ATTRIBUTE, {'anonid': 'button'}).find_element(
> +            By.ANON_ATTRIBUTE, {'anonid': 'button'}).click()

Same here for the element, including other instances below.

::: testing/puppeteer/firefox/firefox_puppeteer/ui/browser/window.py:97
(Diff revision 2)
>              self._navbar = NavBar(lambda: self.marionette, self, navbar)
>  
>          return self._navbar
>  
>      @property
> +    def notification(self):

Also we might have to think about if the notification directly under the browser is correct here. UI wise it is connected to the toolbar, and even only available for the tab where it has been raised. But maybe we should keep the door hanger notifications as is and add the tab-specific notifications under the tab.

::: testing/puppeteer/firefox/firefox_puppeteer/ui/browser/window.py:115
(Diff revision 2)
> +                By.CSS_SELECTOR, '#notification-popup popupnotification')
> +        except NoSuchElementException:
> +            return None  # no notification is displayed
> +        notification_id = notification.get_attribute('id')
> +        return notifications_map[notification_id](
> +            lambda: self.marionette, self, notification)

I would propose that we return a base notification in case the current type is not being supported yet.

::: testing/puppeteer/firefox/firefox_puppeteer/ui/browser/window.py:118
(Diff revision 2)
> +        notification_id = notification.get_attribute('id')
> +        return notifications_map[notification_id](
> +            lambda: self.marionette, self, notification)
> +
> +    def wait_for_notification(self, notification_class=BaseNotification):
> +        """Waits for the specified notification to be displayed."""

nit: we should always document the parameters for the right API documentation.

::: testing/puppeteer/firefox/firefox_puppeteer/ui/browser/window.py:126
(Diff revision 2)
> +                lambda _: self.notification is None,
> +                message='Unexpected notification shown.')
> +        else:
> +            message = '{0} was not shown.'.format(notification_class.__name__)
> +            if notification_class is BaseNotification:
> +                message = 'No notification was shown.'

Not sure why we have to set the message again. The `__name__` property should also be existent for the BaseNotification class.
Blocks: 1272710
No longer depends on: 1272710
Blocks: 1272709
No longer depends on: 1272709
Duplicate of this bug: 1260408
You need to log in before you can comment on or make changes to this bug.