Closed Bug 885015 Opened 13 years ago Closed 13 years ago

Permanent orange on UX branch: TEST-UNEXPECTED-FAIL | TypeError: AddonsMgrListener is undefined, Test Timed Out

Categories

(Firefox :: Toolbars and Customization, defect)

Product:
Firefox
Component:
Toolbars and Customization
Platform:
x86_64
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 25
Tracking Flags:
Tracking Status
firefox24 --- unaffected
firefox25 --- fixed

People

(Reporter: mconley, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Keywords: intermittent-failure, Whiteboard: [Australis:M8][Australis:P1])

Attachments

(3 files, 2 obsolete files)

Patch - just expose IDs rather than widget nodes
1.43 KB, patch
mconley
: review+
Gijs
: checkin+
Details | Diff | Splinter Review
Diff -w for SDK + tests
5.58 KB, patch
Details | Diff | Splinter Review
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1055
358 bytes, text/html
evold
: review+
Details
+++ This bug was initially created as a clone of Bug #885012 +++ I thought there were bugs filed for these, but a quick search hasn't revealed any. TEST-UNEXPECTED-FAIL | tests/test-widget.testNavigationBarWidgets | "Camera / Microphone Access" != "2nd widget" TEST-UNEXPECTED-FAIL | tests/test-widget.testNavigationBarWidgets | "Bookmarks" != "3rd widget" See: https://tbpl.mozilla.org/php/getParsedLog.php?id=24341388&tree=UX
Noticed this in the log: console.log: [CustomizableUI], Searching for widget:a9e44e34-4814-42da-a515-fa0ff3852209@jetpack-3rd in toolbox. console.log: [CustomizableUI], No node for widget:a9e44e34-4814-42da-a515-fa0ff3852209@jetpack-3rd found. console.error: [CustomizableUI] TypeError: widgetNode is undefined -- resource:///modules/CustomizableUI.jsm:523
If I'm understanding this test and error correctly, I think this is related to the add-ons bar work. Gijs, can you confirm?
Flags: needinfo?(gijskruitbosch+bugs)
Adding keyword so this appears in the TBPL bug suggestions.
Keywords: intermittent-failure
(In reply to Mike Conley (:mconley) from comment #2) > If I'm understanding this test and error correctly, I think this is related > to the add-ons bar work. Gijs, can you confirm? Yes and no. Essentially, this error should be fixable regardless of the add-on bar, but if we're removing the add-on bar more stuff will break, and I'd rather wait for a decision + push there than do the same "make this test pass" work twice.
Flags: needinfo?(gijskruitbosch+bugs)
Bumping these to M8, since M7 finished yesterday.
Whiteboard: [Australis:M7] → [Australis:M8]
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
P1 in that we can't land or ship with permaorange. Need to fix or disable test as appropriate.
Whiteboard: [Australis:M8] → [Australis:M8][Australis:P1]
Attached patch Diff -w for SDK tests (obsolete) — — Splinter Review
After applying the patches from bug 749804, the tests do fail quite badly. This fixes most tests, but it also disables the add-on bar UI tests, the "move things to the navbar" test (because they're already in the navbar...) and the 'window close exception' test. The latter is one that I think really actually needs actual fixing in the SDK, also because I see that exception for some add-ons that get automatically moved out of the add-on bar into the palette. I'm also wondering if we should update the SDK code to auto-place items in the navbar/palette itself depending on their width... In any case, I'm confused about what's going on here. It seems there are two paths that will destroy the _symbiont: the unload listener on the frame, and the widget destroy methods. In the test, I'm seeing the unload listener called rather than the widget destroy one, which then confuses this code and makes it throw an exception. I'm not really sure what the deal is with this test and how best to go about fixing it. Matteo, what do you think of what I've done so far, and do you have any ideas about the remaining test?
Attachment #766411 - Flags: feedback?(zer0)
We currently return all the current widgets. Historically, that's not how currentSet works: it includes items that may have been removed or not readded etc. We should reflect that behaviour.
Attachment #766412 - Flags: review?(mconley)
Comment on attachment 766412 [details] [diff] [review] Fix currentSet implementation to use placements array Annnnnd then I did all of this on the plane and only just realized that the *attribute* behaves that way, but the *property* doesn't. And the SDK relies on the attribute, which we no longer support. Sounds like we need to teach the SDK about CustomizableUI, then. :-\
Attachment #766412 - Flags: review?(mconley)
(In reply to :Gijs Kruitbosch from comment #7) > Created attachment 766411 [details] [diff] [review] > Diff -w for SDK tests > > After applying the patches from bug 749804, the tests do fail quite badly. > This fixes most tests, but it also disables the add-on bar UI tests, the > "move things to the navbar" test (because they're already in the navbar...) > and the 'window close exception' test. NB: sigh, so this depended on the patch I just marked obsolete, and will need more work to instead use CustomizableUI to get the list of IDs rather than the currentset attribute.
Actually, this should be enough
Attachment #766412 - Attachment is obsolete: true
Attachment #766684 - Flags: review?(mconley)
Attached patch Diff -w for SDK + tests — — Splinter Review
Annnd this fixes the window close issues I was seeing. Woo! Matteo, does this look decent to you? Then I can put up a github pull req.
Attachment #766411 - Attachment is obsolete: true
Attachment #766411 - Flags: feedback?(zer0)
Attachment #766686 - Flags: feedback?(zer0)
Comment on attachment 766684 [details] [diff] [review] Patch - just expose IDs rather than widget nodes Review of attachment 766684 [details] [diff] [review]: ----------------------------------------------------------------- Yep, cool with me.
Attachment #766684 - Flags: review?(mconley) → review+
Comment on attachment 766684 [details] [diff] [review] Patch - just expose IDs rather than widget nodes Pushed: https://hg.mozilla.org/projects/ux/rev/8ba322cb77ac
Attachment #766684 - Flags: checkin+
Pointer to Github pull-request
Summary: Permanent orange on UX branch: TEST-UNEXPECTED-FAIL | tests/test-widget.testNavigationBarWidgets → Permanent orange on UX branch: TEST-UNEXPECTED-FAIL | TypeError: AddonsMgrListener is undefined, Test Timed Out
Comment on attachment 768026 [details] Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1055 Just a couple minor things need changing.
Attachment #768026 - Flags: review+
Commits pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/a80a7b295856d0142ab8b5edcd5d7e16fdee7100 Bug 885015 - Make jetpack widgets play nice with add-on bar removal. https://github.com/mozilla/addon-sdk/commit/c4fb1780c4e619bd7b383e398a8be76823835afc Merge pull request #1055 from gijsk/885015 Bug 885015 - Make jetpack widgets play nice with add-on bar removal. r=@erikvold
Comment on attachment 766686 [details] [diff] [review] Diff -w for SDK + tests Cancelling feedback request now that this has landed in the add-on SDK git repo.
Attachment #766686 - Flags: feedback?(zer0)
I can see green jetpack tests on the UX branch, hurray!
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M8][Australis:P1] → [Australis:M8][Australis:P1][fixed-in-ux]
status-firefox24: --- → unaffected
status-firefox25: --- → fixed
Target Milestone: --- → Firefox 25
https://hg.mozilla.org/mozilla-central/rev/8ba322cb77ac
Whiteboard: [Australis:M8][Australis:P1][fixed-in-ux] → [Australis:M8][Australis:P1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: