Closed Bug 1783248 Opened 2 years ago Closed 2 years ago

Extend test-verify task with a check to ensure about:config prefs aren't persistently modified by tests

Categories

(Testing :: General, enhancement)

Default
enhancement

Tracking

(firefox113 fixed)

RESOLVED FIXED
113 Branch
Tracking Status
firefox113 --- fixed

People

(Reporter: dholbert, Assigned: jmaher)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Feature-request:

When we run a test in "test-verify" mode (locally with --verify or as part of the test-verify task on TreeHerder), we should make sure that it doesn't persistently modify pref state.

e.g. we could presumably snapshot the about:config pref values before we run the test, and then snapshot them again after the test, and see if there are any changes, and complain if there were.

(If there were, this indicates that the test inadvertently is introducing some statefulness into our test runs and may cause fragile dependencies/flakiness in later tests.)

This would have caught & prevented e.g. bug 1783093 (and the associated intermittent failure) if we'd had this mechanism in place when the associated tests there were checked into the tree.

Flags: needinfo?(jmaher)

Two more notes:
(1) It would also be nice to have a way to opt in to running every test in an entire test-suite in this configuration (with pref-snapshotting just before & after every test). That could help flush out any latent issues like bug 1783093 up-front. (The test-verify configuration would presumably catch new issues, but we would also like to be able to discover & fix preexisting issues of this sort.)

(2) I suspect/assume we'll need to have an "ignore-list" for prefs that we don't care about when comparing before/after pref snapshots. This would include any "non-configuration-impacting" prefs that we know/discover to be harmlessly modified during test-runs (possibly even in the background). As an example: looking through my own about:config data, it looks like e.g. browser.newtabpage.activity-stream.migrationLastShownDate and browser.safebrowsing.provider.google4.lastupdatetime might be candidates for an allow-list, if those prefs get written during test runs.

this is a great idea- I am looking into some work on test-verify in the short term.

It sounds like we want the test harness to support pref snapshot/compare. This will have to be harness by harness.

I think the best way to implement this will be to have something built into special powers that runs at the beginning of every test and end of every test sort of like this:

<test-setup>
beforePrefs = specialPowers.getAllPrefs()
...
<do-test>
...
<test-cleanup>
changedPrefs = specialPowers.diffPrefs(beforePrefs, specialPowers.getAllPrefs())
changedPrefs = removeKnownPrefs(changedPrefs, ignoreList=path/filename.json)
assert changedPrefs == None
<next test...>

Given this implementation, we could ensure that ALL tests are run like this all the time. This could be behind a flag and then it could be only done during test-verify or when requested- I imagine the cost in time/cpu will be minimal to do this. Technically the beforePrefs should be static upon browser startup and the same at the beginning of every test, so that could be called one time.

there are a few harnesses to do this in:
mochitest (and browser-chrome/chrome subharnesses)
xpcshell
reftest
web-platform

That will cover the large majority of our tests

Flags: needinfo?(jmaher)

and here is a try push with this:
https://treeherder.mozilla.org/jobs?repo=try&tier=1%2C2%2C3&revision=1396bc34e4438239aa5c5015289e25c5257b9475

a good list of preferences to sort out. also if one test leaves a preference all subsequent tests do

Blocks: 1816524
Assignee: nobody → jmaher
Attachment #9317476 - Attachment description: WIP: Bug 1783248 - [mochitest] Add notification of changed pref at end of test. → Bug 1783248 - [mochitest] Add notification of changed pref at end of test and error if changed in test-verify. r=ahal!
Status: NEW → ASSIGNED
Pushed by jmaher@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/df7eaa7f8d07 [mochitest] Add notification of changed pref at end of test and error if changed in test-verify. r=ahal
Regressions: 1823840
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 113 Branch
Depends on: 1824270
Regressions: 1824488
Blocks: 1829062
Regressions: 1832358
No longer regressions: 1832358
Blocks: 1832358
Depends on: 1838233
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: