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)
Tracking
(firefox17+ fixed)
RESOLVED
FIXED
Firefox 18
People
(Reporter: philor, Assigned: markh)
References
Details
(Keywords: intermittent-failure)
Attachments
(1 file, 1 obsolete file)
6.40 KB,
patch
|
Gavin
:
review+
Gavin
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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 (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 6•12 years ago
|
||
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 (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 11•12 years ago
|
||
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 (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 16•12 years ago
|
||
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•12 years ago
|
||
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 (Legacy TBPL/Treeherder Robot) |
Comment 19•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #656704 -
Flags: review?(gavin.sharp) → review+
Reporter | ||
Updated•12 years ago
|
status-firefox17:
--- → affected
Assignee | ||
Comment 20•12 years ago
|
||
Assignee | ||
Comment 21•12 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 (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 24•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Updated•12 years ago
|
tracking-firefox17:
--- → +
Updated•12 years ago
|
Attachment #656704 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment hidden (Legacy TBPL/Treeherder Robot) |
Updated•12 years ago
|
Keywords: checkin-needed
Comment 26•12 years ago
|
||
Keywords: checkin-needed
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Updated•12 years ago
|
Keywords: intermittent-failure
Updated•12 years ago
|
Whiteboard: [orange]
Updated•6 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•