1446508 - Improve test infrastructure for policies

Status: RESOLVED FIXED

(Firefox :: Enterprise Policies, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Some ergonomics improvements for the test infrastructure for policies:

- clear runOnce policies at the end of every test (patch already written by Yuki)
- reset and unlock prefs that were set by policies
- make it easier to add tests for simple pref-flipping policies

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1446508 - clear runOnce state after every test.

Review commit: https://reviewboard.mozilla.org/r/229616/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229616/

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1446508 - Move the helper function checkLockedPref to a common location to be used by more tests.

Review commit: https://reviewboard.mozilla.org/r/229618/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229618/

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1446508 - Rewrite test browser_policy_disable_fxscreenshots.js to make it more stable.

Review commit: https://reviewboard.mozilla.org/r/229620/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229620/

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1446508 - Track prefs that had their default values changed, and restore them automatically when restarting the policy engine.

Review commit: https://reviewboard.mozilla.org/r/229622/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229622/

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1446508 - Create test file that makes it easier to add tests for simple policies.

Review commit: https://reviewboard.mozilla.org/r/229624/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229624/

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8960857 [details]
Bug 1446508 - clear runOnce state after every test.

https://reviewboard.mozilla.org/r/229616/#review235344

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8960857 [details]
Bug 1446508 - clear runOnce state after every test.

https://reviewboard.mozilla.org/r/229616/#review235346

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8960858 [details]
Bug 1446508 - Move the helper function checkLockedPref to a common location to be used by more tests.

https://reviewboard.mozilla.org/r/229618/#review235348

::: browser/components/enterprisepolicies/tests/EnterprisePolicyTesting.jsm:60
(Diff revision 1)
> +  checkPolicyPref(prefName, expectedValue, expectedLockedness) {
> +    if (expectedLockedness !== undefined) {
> +      Assert.equal(Preferences.locked(prefName), expectedLockedness, `Pref ${prefName} is correctly locked/unlocked`);
> +    }
> +
> +    Assert.equal(Preferences.get(prefName), expectedValue, `Pref ${prefName} has the correct value`);

Note: I usually dislike using Preferences.jsm, but this patch had several places where I was having to write a switch() based on the variable type, to call either getBoolPref, getIntPref, etc., so it was making it harder to read and more complicated to write. So I just went with Preferences.jsm

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8960858 [details]
Bug 1446508 - Move the helper function checkLockedPref to a common location to be used by more tests.

https://reviewboard.mozilla.org/r/229618/#review235350

::: browser/components/enterprisepolicies/tests/EnterprisePolicyTesting.jsm:57
(Diff revision 1)
> +      Assert.equal(Preferences.locked(prefName), expectedLockedness, `Pref ${prefName} is correctly locked/unlocked`);
> +    }
> +
> +    Assert.equal(Preferences.get(prefName), expectedValue, `Pref ${prefName} has the correct value`);

I kinda thought Preferences.jsm was deprecated?

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8960860 [details]
Bug 1446508 - Track prefs that had their default values changed, and restore them automatically when restarting the policy engine.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229622/diff/1-2/

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8960861 [details]
Bug 1446508 - Create test file that makes it easier to add tests for simple policies.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229624/diff/1-2/

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8960859 [details]
Bug 1446508 - Rewrite test browser_policy_disable_fxscreenshots.js to make it more stable.

https://reviewboard.mozilla.org/r/229620/#review235352

::: browser/components/enterprisepolicies/tests/browser/disable_fxscreenshots/browser_policy_disable_fxscreenshots.js:20
(Diff revision 1)
> -  await BrowserTestUtils.withNewTab("data:text/html,Test", async function() {
> -    // Firefox Screenshots are disabled in tests, so make sure we enable
> -    // it first to ensure that the test is valid.
> -    Services.prefs.setBoolPref(PREF_DISABLE_FX_SCREENSHOTS, false);
> -    await checkScreenshots(true);
> -
> +  // Dynamically toggling the PREF_DISABLE_FX_SCREENSHOTS is very finicky, because
> +  // that pref is being watched, and it makes the Firefox Screenshots system add-on
> +  // to start or stop, causing intermittency.
> +  //
> +  // Firefox Screenshots is disabled by default on tests (in prefs_general.js).
> +  // What we do here to test this policy is to set the to false on this specific

missing word(s): "set the to"

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8960860 [details]
Bug 1446508 - Track prefs that had their default values changed, and restore them automatically when restarting the policy engine.

https://reviewboard.mozilla.org/r/229622/#review235356

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8960861 [details]
Bug 1446508 - Create test file that makes it easier to add tests for simple policies.

https://reviewboard.mozilla.org/r/229624/#review235358

Comment 15

[:Felipe Gomes (needinfo for replies!)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0e0fab8f965d848b25446f3f5d3c97b971ecba1

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4642c1e1d23
clear runOnce state after every test. r=felipe
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0a1bf2e04d2
Move the helper function checkLockedPref to a common location to be used by more tests. r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc026f577122
Rewrite test browser_policy_disable_fxscreenshots.js to make it more stable. r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/a92b420e1f7f
Track prefs that had their default values changed, and restore them automatically when restarting the policy engine. r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c929665312f
Create test file that makes it easier to add tests for simple policies. r=MattN

Comment 17

[:Felipe Gomes (needinfo for replies!)]

[Tracking Requested - why for this release]:
Improvements to the tests for enterprise policies

Comment 18

[:Felipe Gomes (needinfo for replies!)]

[Tracking Requested - why for this release]:
Sorry, meant to set the flag for 60, not 59

Comment 19

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/a4642c1e1d23
https://hg.mozilla.org/mozilla-central/rev/a0a1bf2e04d2
https://hg.mozilla.org/mozilla-central/rev/cc026f577122
https://hg.mozilla.org/mozilla-central/rev/a92b420e1f7f
https://hg.mozilla.org/mozilla-central/rev/4c929665312f

Comment 20

[:Felipe Gomes (needinfo for replies!)]

Attachment: Rolled-up patch for beta. r=MattN

Approval Request Comment
[Feature/Bug causing the regression]: Test improvements only, nothing is part of the product
[User impact if declined]: Test helpers that exist in mozilla-central won't exist in mozilla-beta, making it harder to uplift new tests
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: tests only
[String changes made/needed]: none

Comment 21

[Julien Cristau [:jcristau]]

This can go in as a test-only change.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

The rollup patch doesn't apply to Beta at the moment, presumably due to missing uplift dependencies. Please put checkin-needed-beta back on the bug when all the necessary bugs have been uplifted.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/45249cb4b63e