Update CustomizableUI tests to yield on CustomizeMode.reset instead of gCustomizeMode.resetting

RESOLVED FIXED in Firefox 48

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: jaws, Assigned: gasolin, Mentored)

Tracking

(Blocks: 2 bugs)

29 Branch
Firefox 48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [Australis:P-][good first bug][lang=js])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

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

2 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@gmail.com
Whiteboard: [Australis:P-] → [Australis:P-][good first bug][lang=js]
(Assignee)

Comment 2

2 years ago
take it a try
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
(Assignee)

Comment 3

2 years ago
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?

Comment 4

2 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

2 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

2 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

2 years ago
Created attachment 8737031 [details]
MozReview Request: Bug 969443 - Update CustomizableUI tests to yield on CustomizeMode.reset; r?Gijs

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

2 years ago
I re-push to Mozreview and it seems work now. Please help review the patch, thanks!
Flags: needinfo?(gasolin)

Updated

2 years ago
Attachment #8737031 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 9

2 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

2 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

2 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. :-\
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.

Comment 13

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/45de51023c1f
(Assignee)

Comment 14

2 years ago
thanks!
Status: ASSIGNED → RESOLVED
Last Resolved: 2 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

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/45de51023c1f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
(Assignee)

Comment 17

2 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.