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)

29 Branch
defect
Not set
normal

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();`.
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]
take it a try
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Attached file 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?
(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?
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)
(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)
I re-push to Mozreview and it seems work now. Please help review the patch, thanks!
Flags: needinfo?(gasolin)
Attachment #8737031 - Flags: review?(gijskruitbosch+bugs) → review+
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!
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.
(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. :-\
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.
thanks!
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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 → ---
Status: REOPENED → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/45de51023c1f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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.