Closed Bug 786085 Opened 12 years ago Closed 12 years ago

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

Categories

(Firefox Graveyard :: SocialAPI, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(firefox17+ fixed)

RESOLVED FIXED
Firefox 18
Tracking Status
firefox17 + fixed

People

(Reporter: philor, Assigned: markh)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

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
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.
Attached patch Use a mock alerts service (obsolete) — Splinter Review
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 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-
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)
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+
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?
https://hg.mozilla.org/mozilla-central/rev/1b0b56afa33a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Attachment #656704 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [orange]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: