Closed Bug 1446508 Opened 6 years ago Closed 6 years ago

Improve test infrastructure for policies

Categories

(Firefox :: Enterprise Policies, defect)

Product:
Firefox
Component:
Enterprise Policies
Version:
60 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox60 + fixed
firefox61 --- fixed

People

(Reporter: Felipe, Assigned: Felipe)

References

Details

Attachments

(6 files)

Bug 1446508 - clear runOnce state after every test.
59 bytes, text/x-review-board-request
Felipe
: review+
MattN
: review+
Details
Bug 1446508 - Move the helper function checkLockedPref to a common location to be used by more tests.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1446508 - Rewrite test browser_policy_disable_fxscreenshots.js to make it more stable.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1446508 - Track prefs that had their default values changed, and restore them automatically when restarting the policy engine.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1446508 - Create test file that makes it easier to add tests for simple policies.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Rolled-up patch for beta. r=MattN
36.68 KB, patch
Felipe
: review+
Details | Diff | Splinter Review 
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 on attachment 8960857 [details]
Bug 1446508 - clear runOnce state after every test.

https://reviewboard.mozilla.org/r/229616/#review235344
Attachment #8960857 - Flags: review?(felipc) → review+
Comment on attachment 8960857 [details]
Bug 1446508 - clear runOnce state after every test.

https://reviewboard.mozilla.org/r/229616/#review235346
Attachment #8960857 - Flags: review+
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 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?
Attachment #8960858 - Flags: review?(MattN+bmo) → review+
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"
Attachment #8960859 - Flags: review?(MattN+bmo) → review+
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
Attachment #8960860 - Flags: review?(MattN+bmo) → review+
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
Attachment #8960861 - Flags: review?(MattN+bmo) → review+
Pushed by felipc@gmail.com:
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
[Tracking Requested - why for this release]:
Improvements to the tests for enterprise policies
tracking-firefox59: --- → ?
[Tracking Requested - why for this release]:
Sorry, meant to set the flag for 60, not 59
status-firefox60: --- → affected
tracking-firefox59: ? → ---
tracking-firefox60: --- → ?
Depends on: 1429177
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
Attachment #8961601 - Flags: review+
Attachment #8961601 - Flags: approval-mozilla-beta?
This can go in as a test-only change.
Whiteboard: [checkin-needed-beta]
Attachment #8961601 - Flags: approval-mozilla-beta?
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.
Flags: needinfo?(felipc)
Whiteboard: [checkin-needed-beta]
Flags: needinfo?(felipc)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: