Test: filter_buttons doesn't cleanup preferences

RESOLVED FIXED in Firefox 43

Status

RESOLVED FIXED
3 years ago
6 months ago

People

(Reporter: Honza, Assigned: Honza)

Tracking

42 Branch
Firefox 43
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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
Created attachment 8654081 [details] [diff] [review]
bug1199623-1.patch

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)
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
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+
Created attachment 8654138 [details] [diff] [review]
bug1199623-2.patch

(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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/053f499b510f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.