1815425 - conditioned profile variant is expiring on March 01, 2023

Status: NEW

(Testing :: General, task, P1)

Description

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

we need to renew the expiration of conditioned profile test variant (proposal.)

We turned this on almost 9 months ago with the intention of catching regressions (i.e. bug 1750188). So far there have been no regressions found and fixed in the product that were a by product of the condprof tests.

We run the test harnesses mochitest-plain and xpcshell (skipped a good bit of the tests due to failures) on linux/win10 shippable (view on central.)

Other data would be good to know if there are bugs we are aware of that seem to be related to an existing profile and not able to reproduce on a fresh profile.

I see a few options ahead:

1. adjust the tests we run by adding or removing from the list we run
2. adjust the profile we are using as it might not be finding issues
3. turn off the tests
4. keep what we have for another 6 months and revisit in August

I am needinfo'ing a few folks that might have info to add here or a preference of next steps- please redirect to others that would have good information.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Joel Maher ( :jmaher ) (UTC -8) from comment #0)

We run the test harnesses mochitest-plain and xpcshell (skipped a good bit of the tests due to failures) on linux/win10 shippable (view on central.)

For others not familiar (I was not), runxpcshelltests.py explicitly has logic that knows to use the conditioned profile directory so even though normally xpcshell tests get their own freshly created temporary directory to use when do_get_profile is called, the xpcshell tests running with a conditioned profile get a freshly created copy of the conditioned profile.

In terms of the bug 1750188 regression that the condprof did not catch, it's my understanding that RemoteSettings was involved, and my general impression is that:

• RemoteSettings does like to run in xpcshell even though most tests probably don't want that.
  • For example, I was just able to see RemoteSettings doing IDB database stuff by running MOZ_LOG=IndexedDB:5 ./mach xpcshell-test dom/workers/test/xpcshell/test_remoteworker_launch_new_process.js
• Usually xpcshell tests are pretty fast so many tests will race RemoteSettings doing stuff, so even if condprof might have detected the regression in bug 1750188, if we had added an event loop spin for some number of seconds in do_get_profile when using a conditioned profile, it's possible that condprof could have caught that maybe? I do not understand the specific nuances of that regression at this time.

I see a few options ahead:

1. adjust the tests we run by adding or removing from the list we run

At least for the cases around the condprof-like , I think we definitely could still benefit from the condprof mechanism or a variation on it.

For DOM Storage related APIs, we've had 2 instances where bad things happened that could have been caught via a condprof-like mechanism.

1. Bug 1549362 - Local Storage NextGen (LSNG) broke nightly!
   • Note that test coverage was added subsequently for a specific canned profile, which is the general approach taken for cases like this, but the general problem is these are manually created test-cases that require good hygiene practices to proactively create and won't really catch regressions where people make changes not realizing that they're changing things.
2. Bug 1458320 - Some IndexedDB databases broke!
   • This is an example where something was changed without realizing the impact or the need to add canned profile tests, but where something like condprof could have caught it.

Bug 1549417 was filed in response to the first of the two bugs and maybe could be duped to whatever bug allows for specific, intentional coverage for IndexedDB and related storage APIs.

I think we would want to introduce either a new class of tests or a special mode of running an existing class of tests. Most of our existing tests assume there is no relevant prior storage state and through some combination of pre-emptive cleanup and trying to clean up after themselves, they will usually break 100% of the time or not notice the impact of a pre-existing profile.

It's quite common in testing frameworks like our marionette harness to have setup and teardown methods. While the semantics wouldn't be exactly that, you could imagine we might have a testing convention/mini-framework with 2 required methods:

• putYourDataInTheProfile(): This method is called to put data in the conditioned profile.
  • For IndexedDB we'd create a database with a fixed name and put some known set of fixed values inside it.
• checkYourDataInTheProfile(): This method is called to validate data that is guaranteed to have been stored previously in the profile via the putYourDataInTheProfile method. For IDB we'd validate the data above. We return a list of observed problems that could include:
  • "broken": We couldn't do the thing we were trying to do! Functionality is broken.
  • "missing-data": The data doesn't seem like it's there anymore, that seems bad!
  • "corrupt-data": The data seemed to be there but also seemed to be corrupt.
  • "downgrade-detected": Best-effort signaling for cases where It looks like we're experiencing a downgrade and that might explain any problems we're seeing.
• optional analyzeYourDataOnline(): This method is called with the browser active, allowing full access to XPCOM to derive a JSON object that test automation would make sure gets reported to a dashboard or a bug or whatever. This could be automatically run by automation before and after the test, although it might be necessary for the version that runs before the test to be run in a separate session since it could have side-effects. (This would only be necessary if a call to checkYourDataInTheProfile indicated problems though.)
• optional analyzeYourDataOffline(): Similar idea as the above but the idea would be that in this case the browser is explicitly not running and we're in some kind of alternate runtime environment that we use to poke at things, like we're in a python runtime or a standalone xpcshell instance.

Additional things that would probably need to be addressed:

• test versioning: It probably would make sense to have the test define a version number for itself and we guarantee that we only run your test against the checkYourDataInTheProfile against the same version of putYourDataInTheProfile. It's obviously possible to do something more complicated with upgrades and such, but that seems like a lot of complication for no benefit. This would mean that tests like this would need to land / be updated separate from and prior to any patch stacks that would be making concerning changes, but that seems reasonable.
• origins / test partitioning: In order to avoid having tests colliding or needing their own profiles, it would probably make sense for each test to get its own origin name which also bakes in the version number. That would be expressed to the test or inherent in how it's run.

It's not immediately clear what the best existing framework would be for things like this. Marionette and "browser" mochitests would have the best ability to do the diagnostic introspection for "analyzeYourData", but "plain" mochitests and WPT would probably be more ergonomic for the tests proper if you were expecting to write a lot of tests. For things like the storage APIs, we'd probably only have one test per major API/subsystem (QuotaManager, LocalStorage NG, Cache API, ServiceWorkers, OPFS), and each would probably have their own single introspection debugging mechanism. And I guess we'd also want to actually move the introspection into something that an "about:" UI could use in the field, so probably from the test's perspective that would end up just being a single (async) XPCOM call against the origin in question for the Online case.

Comment 2

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

thanks :asuth, for now I will keep our existing variant while actively working to change what we run. These are great examples and you highlight many cases/options to consider.

Comment 3

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

I will extend this in bug 1815516 and keep this bug for tracking what tests to run, etc