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

Status: RESOLVED FIXED

(Testing :: General, enhancement)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

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.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: Bug 1783248 - [mochitest] Add notification of changed pref at end of test and error if changed in test-verify. r=ahal!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
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

Comment 6

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/df7eaa7f8d07