1199623 - Test: filter_buttons doesn't cleanup preferences

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: 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

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: 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

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/053f499b510f

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/053f499b510f