Closed
Bug 969443
Opened 10 years ago
Closed 8 years ago
Update CustomizableUI tests to yield on CustomizeMode.reset instead of gCustomizeMode.resetting
Categories
(Firefox :: Toolbars and Customization, defect)
Tracking
()
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jaws, Assigned: gasolin, Mentored)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [Australis:P-][good first bug][lang=js])
Attachments
(2 files)
CustomizeMode.reset returns a Task, which can be yielded on. There are a few tests that do `yield waitForCondition(function() !gCustomizeMode.resetting);`. This shouldn't be necessary, as the test can just call `yield gCustomizeMode.reset();`.
Comment 1•8 years ago
|
||
Specifically: https://dxr.mozilla.org/mozilla-central/search?q=yield+waitForCondition+resetting+path%3Acustomizableui&redirect=false&case=false browser/components/customizableui/test/browser_1089591_still_customizable_after_reset.js browser/components/customizableui/test/browser_923857_customize_mode_event_wrapping_during_reset.js browser/components/customizableui/test/browser_938980_navbar_collapsed.js browser/components/customizableui/test/browser_970511_undo_restore_default.js
Mentor: gijskruitbosch+bugs
Whiteboard: [Australis:P-] → [Australis:P-][good first bug][lang=js]
Assignee | ||
Comment 3•8 years ago
|
||
I modified the tests with `yield gCustomizeMode.reset()` Most file works well but there are some fails in browser_970511_undo_reset_defaults.js comment out these three places make all test works 1. https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/browser_970511_undo_restore_default.js#28 2. https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/browser_970511_undo_restore_default.js#40 3. https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/browser_970511_undo_restore_default.js#44 I suspect its related to the timing of `undoResetButton.click();` function, could you help me figure it out?
Comment 4•8 years ago
|
||
(In reply to Fred Lin [:gasolin] from comment #3) > Created attachment 8735784 [details] > mozreview > > I modified the tests with `yield gCustomizeMode.reset()` > > Most file works well but there are some fails in > browser_970511_undo_reset_defaults.js > > comment out these three places make all test works > 1. > https://dxr.mozilla.org/mozilla-central/source/browser/components/ > customizableui/test/browser_970511_undo_restore_default.js#28 > 2. > https://dxr.mozilla.org/mozilla-central/source/browser/components/ > customizableui/test/browser_970511_undo_restore_default.js#40 > 3. > https://dxr.mozilla.org/mozilla-central/source/browser/components/ > customizableui/test/browser_970511_undo_restore_default.js#44 > > I suspect its related to the timing of `undoResetButton.click();` function, > could you help me figure it out? So undoResetButton.click() will "undo" the reset, while gCustomizeMode.reset() will re-execute a reset. Running both at the same time will lead to indeterminate / broken results. The UI is set up so the user shouldn't be able to do this themselves, but it seems there are no guards in the code against this - there probably should be, but that doesn't need to be addressed in this bug. Instead, to fix this file, you can replace both the undoResetButton.click() call and the yield waitForCondition(...) on line 24-25 with just: yield gCustomizeMode.undoReset() Does that make sense?
Assignee | ||
Comment 5•8 years ago
|
||
Thanks it does fix the tests! patch updated BTW, I cant set you as reviewer because bugzilla shows error that you don't except review. https://reviewboard.mozilla.org/r/42933/
Flags: needinfo?(gijskruitbosch+bugs)
Comment 6•8 years ago
|
||
(In reply to Fred Lin [:gasolin] from comment #5) > Thanks it does fix the tests! patch updated > > > BTW, I cant set you as reviewer because bugzilla shows error that you don't > except review. > https://reviewboard.mozilla.org/r/42933/ Yes, sorry - I was away on holiday, so then I don't accept review requests. If I did, people might be waiting a long time, so it's a (clumsy!) way of avoiding disappointment, which doesn't really work well because we don't have a good system to suggest you alternative reviewers... anyway, I'm back now, so I could review it now - but it seems the reviewboard item is still private and so I can't see it yet...
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(gasolin)
Assignee | ||
Comment 7•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/42933/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42933/
Attachment #8737031 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 8•8 years ago
|
||
I re-push to Mozreview and it seems work now. Please help review the patch, thanks!
Flags: needinfo?(gasolin)
Updated•8 years ago
|
Attachment #8737031 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 9•8 years ago
|
||
Comment on attachment 8737031 [details] MozReview Request: Bug 969443 - Update CustomizableUI tests to yield on CustomizeMode.reset; r?Gijs https://reviewboard.mozilla.org/r/42933/#review40297 LGTM!
Comment 10•8 years ago
|
||
FWIW, seems you requested a trypush but no tests? It's normally a good idea to include mochitest-bc and mochitest-e10s-bc, at least, when checking browser changes (especially to the tests themselves!). I post-triggered them this time.
Comment 11•8 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #10) > FWIW, seems you requested a trypush but no tests? It's normally a good idea > to include mochitest-bc and mochitest-e10s-bc, at least, when checking > browser changes (especially to the tests themselves!). I post-triggered them > this time. ... though that doesn't seem to actually work. :-\
Comment 12•8 years ago
|
||
I was able to trigger the tests. I was actually a bit overzealous and triggered ones for Windows and OS X too, but I've cancelled those now.
Assignee | ||
Comment 14•8 years ago
|
||
thanks!
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment 15•8 years ago
|
||
Hi Fred, bugs will be marked as resolved as soon they're merged into mozilla-central, automagically. Mozilla-central is our trunk, thus authoritative of what gets shipped and built into a Nightly release. In other words: you don't need to do a thing, just land the patch ;-) Thanks!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•8 years ago
|
Status: REOPENED → ASSIGNED
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/45de51023c1f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Assignee | ||
Comment 17•8 years ago
|
||
Nice to know we can do most things automatically, Thanks!
You need to log in
before you can comment on or make changes to this bug.
Description
•