The default bug view has changed. See this FAQ.

Intermittent browser_notifications.js | failed to find the notification, | Found an unexpected alert:alert at the end of test run

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: philor, Assigned: markh)

Tracking

({intermittent-failure})

Trunk
Firefox 18
x86
Windows XP
intermittent-failure
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17+ fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
https://tbpl.mozilla.org/php/getParsedLog.php?id=14749819&tree=Mozilla-Inbound
Rev3 WINNT 5.1 mozilla-inbound debug test mochitest-other on 2012-08-27 07:39:45 PDT for push febe1ad166da
slave: talos-r3-xp-016

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/social/test/browser/browser_notifications.js | failed to find the notification popup
Stack trace:
    JS frame :: chrome://mochitests/content/browser/toolkit/components/social/test/browser/browser_notifications.js :: <TOP_LEVEL> :: line 119
    JS frame :: chrome://mochikit/content/browser-test.js :: <TOP_LEVEL> :: line 500

TEST-INFO | chrome://mochitests/content/browser/toolkit/components/social/test/browser/browser_notifications.js | sub-test testNotificationCallback complete
WARNING: Nv3DVStreaming CoCreateInstance failed (disabled).: file e:/builds/moz2_slave/m-in-w32-dbg/build/gfx/layers/d3d9/Nv3DVUtils.cpp, line 53
INFO TEST-END | chrome://mochitests/content/browser/toolkit/components/social/test/browser/browser_notifications.js | finished in 663ms
TEST-INFO | checking window state
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/social/test/browser/browser_notifications.js | Found an unexpected alert:alert at the end of test run

https://tbpl.mozilla.org/php/getParsedLog.php?id=14756749&tree=Mozilla-Inbound
Rev3 WINNT 5.1 mozilla-inbound opt test mochitest-other on 2012-08-27 14:42:21 PDT for push 807627473028
slave: talos-r3-xp-062

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/social/test/browser/browser_notifications.js | failed to find the notification popup
Stack trace:
    JS frame :: chrome://mochitests/content/browser/toolkit/components/social/test/browser/browser_notifications.js :: <TOP_LEVEL> :: line 119
    JS frame :: chrome://mochikit/content/browser-test.js :: <TOP_LEVEL> :: line 500

TEST-INFO | chrome://mochitests/content/browser/toolkit/components/social/test/browser/browser_notifications.js | sub-test testNotificationCallback complete
INFO TEST-END | chrome://mochitests/content/browser/toolkit/components/social/test/browser/browser_notifications.js | finished in 73ms
TEST-INFO | checking window state
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/social/test/browser/browser_notifications.js | Found an unexpected unknown window at the end of test run
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
findPopup() is not robust - it uses polling with a hardcoded limit of 50 attempts to find the right popup (if the chrome document doesn't load quickly enough, it will fail).

A more robust test would be to use the mock alerts service implementation in notification_common.js to avoid testing the actual alerts implementation, and focus on testing the Social-related aspects of the functionality.
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 11

5 years ago
Created attachment 656360 [details] [diff] [review]
Use a mock alerts service

It seems quite hard to reuse the existing mock for a number of reasons, so I just duplicated what we need.

try run at https://tbpl.mozilla.org/?tree=Try&rev=ff83089b97cd
Assignee: nobody → mhammond
Attachment #656360 - Flags: review?(gavin.sharp)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment on attachment 656360 [details] [diff] [review]
Use a mock alerts service

>diff --git a/toolkit/components/social/test/browser/browser_notifications.js b/toolkit/components/social/test/browser/browser_notifications.js

>+function force_prompt(allow) {

I know you just copied this stuff, but I don't really understand the name of the function, or why it sets the "notification.prompt" prefs - those seem to be specific to "desktop notifications" which aren't relevant here. You also don't need the enablePrivilege calls. I'd make this just:

function replaceAlertsService() {
>+  Components.manager.QueryInterface(Components.interfaces.nsIComponentRegistrar)
>+            .registerFactory(FAKE_CID, "",
>+                             ALERTS_SERVICE_CONTRACT_ID,
>+                             factory)
>+}

function restoreAlertsService()
>+  Components.manager.QueryInterface(Components.interfaces.nsIComponentRegistrar)
>+            .registerFactory(ALERTS_SERVICE_CID, "",
>+                             ALERTS_SERVICE_CONTRACT_ID,
>+                             null);
>+}

>+function force_click_on_notification(val) {
>+function is_feature_enabled() {

These are unused so you shouldn't copy them.

> function test() {
>   waitForExplicitFinish();
> 
>+  let cbPreTest = function(cb) {
>+    force_prompt(true);
>+    cb();
>+  };
>   let cbPostTest = function(cb) {
>+    reset_prompt();
>     SocialService.removeProvider(TEST_PROVIDER_ORIGIN, function() {cb()});
>   };
>+  runTests(tests, cbPreTest, cbPostTest);

IIRC this pretest and posttest are run before/after every individual test - it would be sufficient to just call replaceAlertsService once here before runTests(), and then registerCleanupFunction(restoreAlertsService).

Looks good otherwise; I assume you've tested that the test still fails/passes appropriately with these changes.
Attachment #656360 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 17

5 years ago
Created attachment 656704 [details] [diff] [review]
updated based on review

Changes as requested.

> Looks good otherwise; I assume you've tested that the test still 
> fails/passes appropriately with these changes.

Not sure what you mean here - the test still passes and eyeballing the test output shows all the test checks are actually made.  I posted a URL to the try run of the old patch and a new try run with this patch is at https://tbpl.mozilla.org/?tree=Try&rev=db8d019276b0
Attachment #656360 - Attachment is obsolete: true
Attachment #656704 - Flags: review?(gavin.sharp)
Comment hidden (Treeherder Robot)
I mean that it's worth checking that if you break the notification functionality being tested intentionally, the test reports a failure and does so in a reasonably illustrative way. Always worth testing that in addition to testing that the test still passes when making test changes.
Attachment #656704 - Flags: review?(gavin.sharp) → review+
(Reporter)

Updated

5 years ago
status-firefox17: --- → affected
(Assignee)

Comment 20

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b0b56afa33a
(Assignee)

Comment 21

5 years ago
Comment on attachment 656704 [details] [diff] [review]
updated based on review

[Approval Request Comment]
This is a test-only fix for an intermittent orange.
Bug caused by (feature/regressing bug #): 774506
User impact if declined: moar orange.
Testing completed (on m-c, etc.): landed on inbound.
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None
Attachment #656704 - Flags: approval-mozilla-aurora?
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
https://hg.mozilla.org/mozilla-central/rev/1b0b56afa33a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
tracking-firefox17: --- → +
Attachment #656704 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment hidden (Treeherder Robot)
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/0b5de274b868
status-firefox17: affected → fixed
Keywords: checkin-needed
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Keywords: intermittent-failure
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.