Closed
Bug 1199623
Opened 9 years ago
Closed 9 years ago
Test: filter_buttons doesn't cleanup preferences
Categories
(DevTools :: Console, defect)
Tracking
(firefox43 fixed)
RESOLVED
FIXED
Firefox 43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: Honza, Assigned: Honza)
Details
Attachments
(1 file, 1 obsolete file)
1.81 KB,
patch
|
Honza
:
review+
|
Details | Diff | Splinter Review |
The following test: /browser/devtools/webconsole/test/browser_webconsole_bug_601667_filter_buttons.js ...is checking filters in the Console panel. Changing the filters is also modifying preferences in the user profile, but the test doesn't clean up them and new values can influence other tests running after. Honza
Assignee | ||
Comment 1•9 years ago
|
||
Try push first: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ac898a6b0f5 Panos, this issue has affected my other tests, what do you think about the patch? Honza
Attachment #8654081 -
Flags: review?(past)
Updated•9 years ago
|
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Comment 2•9 years ago
|
||
Comment on attachment 8654081 [details] [diff] [review] bug1199623-1.patch Review of attachment 8654081 [details] [diff] [review]: ----------------------------------------------------------------- This works for me, but we can make it even simpler. ::: browser/devtools/webconsole/test/browser_webconsole_bug_601667_filter_buttons.js @@ +5,5 @@ > > "use strict"; > > const TEST_URI = "http://example.com/"; > +const FILTER_PREF_DOMAIN = "devtools.webconsole.filter"; Add the trailing period here to make sure string concatenation works later. @@ +29,5 @@ > hud = hudId = hudBox = null; > }); > > +function savePrefs() { > + var branch = PrefService.getBranch(FILTER_PREF_DOMAIN); You can use the simpler Services.prefs.getBranch() here. @@ +35,5 @@ > + var children = branch.getChildList("", arrayDesc); > + for (var i = 0; i < children.length; i++) { > + var p = children[i]; > + try { > + prefs[p] = Services.prefs.getBoolPref(FILTER_PREF_DOMAIN + p); Since you are using a branch here, you can simplify this a bit: let children = branch.getChildList(""); for(let child of children) { prefs[child] = branch.getBoolPref(child); } @@ +46,5 @@ > + for (var p in prefs) { > + try { > + Services.prefs.setBoolPref(FILTER_PREF_DOMAIN + p, prefs[p]); > + } catch (err) { > + } You could use a pref branch here or not (up to you), but please don't use try/catch in tests. If something fails we want to know ASAP.
Attachment #8654081 -
Flags: review?(past) → review+
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Panos Astithas [:past] from comment #2) > Comment on attachment 8654081 [details] [diff] [review] > bug1199623-1.patch > > Review of attachment 8654081 [details] [diff] [review]: > ----------------------------------------------------------------- All done, thanks for the review Panos! New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eca73c0cdf2e Honza
Attachment #8654081 -
Attachment is obsolete: true
Attachment #8654138 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 5•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/053f499b510f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•