Closed Bug 1082130 Opened 10 years ago Closed 10 years ago

browser_967000_button_sync|tabview.js fails when running as a standalone directory

Categories

(Firefox :: Sync, defect)

defect
Not set
normal
Points:
1

Tracking

()

RESOLVED FIXED
Firefox 36
Iteration:
36.1

People

(Reporter: jmaher, Assigned: alexbardas)

References

Details

Attachments

(1 file)

in bug 992911, we are looking to enable tests where we run a fresh browser instance per directory.  Usually what happens is that a few tests fail because they accidentally depend on the state of the browser from an earlier test.

In a try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6d8041d9592d

you can see that bc1 is orange across the board and for the most part it is the same tests failing on each platform:
09:35:02 INFO - 1230 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_967000_button_sync.js | A promise chain failed to handle a rejection: - at chrome://browser/content/aboutaccounts/aboutaccounts.js:299 - TypeError: window.location is null
09:35:02 INFO - 1234 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_967000_button_tabView.js | A promise chain failed to handle a rejection: - at chrome://browser/content/aboutaccounts/aboutaccounts.js:299 - TypeError: window.location is null

I am able to reproduce this locally while running:
./mach mochitest-browser browser/components/customizableui/test/browser_967000_button_sync.js

The errors here are sort of cryptic, is there a chance that this is an easy fix or you could give some guidance to me to fix this problem?
This looks like an actual bug in sync's about:accounts not handling what happens if the tab has been closed by the time that the promise resolves. Alex, can you help move this forward?
Component: Toolbars and Customization → Sync
Flags: needinfo?(abardas)
Note that bug 991057 fixed a similar problem, but it looks like that might have got lost in subsequent changes - just below the failing block we have:

    // tests in particular might cause the window to start closing before
    // getSignedInUser has returned.
    if (window.closed) {
      return;
    }

It looks like this just needs to be moved to the top of the block before we start introspecting window.location etc.
Attached patch patch v1Splinter Review
It's what Mark suggested. What I find strange is that I definitely ran only the tests for this file when I worked on the entrypoint bug.

Can it also be a fxAccount regression, taking longer than usual to respond?
Attachment #8504381 - Flags: review?(mhammond)
Flags: needinfo?(abardas)
(In reply to Alex Bardas :alexbardas from comment #3)
> Created attachment 8504381 [details] [diff] [review]
> patch v1
> 
> It's what Mark suggested. What I find strange is that I definitely ran only
> the tests for this file when I worked on the entrypoint bug.
> 
> Can it also be a fxAccount regression, taking longer than usual to respond?

The tests that are failing aren't the tests for this file, they're the toolbar button tests which only landed very recently. They've just exposed this bug, that's all.
Comment on attachment 8504381 [details] [diff] [review]
patch v1

Review of attachment 8504381 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but please make sure it does indeed fix the tests mentioned by Gijs before landing (otherwise we might find there is something else we need to do...)
Attachment #8504381 - Flags: review?(mhammond) → review+
I verified it locally if that helps!
(In reply to Joel Maher (:jmaher) from comment #6)
> I verified it locally if that helps!

Ship it! :)
> Looks good, but please make sure it does indeed fix the tests mentioned by
> Gijs before landing (otherwise we might find there is something else we need
> to do...)

When I tried to run the tests only for ..._sync.js, 2 of them failed. No fails for ..._tabView.js though.

Also, when I locally run the tests for the whole directory: without this patch I get 2 failed tests, with it 0 failed tests.

I'll push to try run to see if it's fixing the problem.
Assignee: nobody → abardas
Status: NEW → ASSIGNED
Keywords: checkin-needed
Points: --- → 1
https://hg.mozilla.org/mozilla-central/rev/54b2b11d1895
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Iteration: --- → 36.1
Flags: qe-verify?
Flags: firefox-backlog+
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.