1773583 - ExperimentFakes.manager() should not replace addEnrollment with an async stub

Status: RESOLVED FIXED

(Firefox :: Nimbus Desktop Client, task, P1)

Description

[Dan Mosedale (:dmosedale, :dmose)]

After discussion with Barret, I removed an assertion over in bug 1748214 where test_ExperimentManager_enroll.js was testing one of the NimbusTestUtils fixtures (the validation functions). The reason was that the validation functions in NimbusTestUtils only support variables that are inline in FeatureManifest.yaml, not ones that where the schema is merely referenced from FeatureManifest.yaml.

A few questions that may provoke some code cleanup:

• are there other places where we're testing the validation code in the text fixtures rather than the validation code which is actually used in product?
• what should happen to the validation code in NimbusTestUtils? Options include removing it and ensuring it's only used to test the production validation code.

Comment 1

[Barret Rennie [:barret] (they/them)]

NimbusTestUtils.jsm is a set of utilities for writing tests using the Nimbus desktop SDK.

ExperimentFakes.manager() creates a new ExperimentManager with a patched ExperimentStore:

    sandbox.stub(manager.store, "addEnrollment").callsFake(async enrollment => {
      await ExperimentTestUtils.validateEnrollment(enrollment);
      return origAddExperiment(enrollment);
    });

This replaces manager.store.addEnrollment with an async function that that validates the enrollment against a schema before calling the original enrollment function.

However, ExperimentStore.addEnrollment is a sync method!

This leads to a bunch of annoyance. You can not write a test like the following:

const manager = ExperimentFakes.manager();
manager.enroll(/* ... */);

/* do something with our enrolled experiment */

We cannot assume that we are enrolled here because ExperimentManager.enroll is not async and can't await the call to ExperimentManager.addEnrollment. Therefore we have additional helpers to work around this. ExperimentFakes.enrollmentHelper:

const manager = ExperimentFakes.manager();
let {
  enrollmentPromise,
  doExperimentCleanup,
} = ExperimentFakes.enrollmentHelper(recipe, { manager });

await enrollmentPromise;

/* do something with our enrolled experiment */

await doExperimentCleanup();

The enrollment helper waits for an event from the ExperimentStore (update:${slug}). However, if something about your test is wrong, you won't see any errors from addEnrollment (due to it being a promise that is not being awaited) and enrollmentPromise will never resolve and your test will hang with no information. This is a very frustrating experience we'd like to avoid.

We would like to remove the stubbing of addEnrollment from ExperimentFakes.manager, which means we can also get rid of the enrollmentPromise returned from enrollmentHelper. The end result of this will be that our tests are closer to how Nimbus actually works in prod and they will be easier to write.

Comment 2

[Charlie Humphreys [:charlie] [:jeddai]]

Attachment: Bug 1773583 - remove stub of enrollmentPromise from nimbus' ExperimentFakes r=barret

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2c5a2aba1d2
remove stub of enrollmentPromise from nimbus' ExperimentFakes r=barret

Comment 4

[Marian-Vasile Laza]

Backed out changeset d2c5a2aba1d2 (Bug 1773583) for causing bc failures on browser_asrouter_remoteimages.js.
Backout link
Push with failures <--> bc9
Failure Log

Comment 5

[Charlie Humphreys [:charlie] [:jeddai]]

I missed that test while making changes -- it's been updated in Phabricator now!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/54c8b5fb832f
remove stub of enrollmentPromise from nimbus' ExperimentFakes r=barret

Comment 7

[Sandor Molnar[:smolnar]]

Backed out changeset 54c8b5fb832f (bug 1773583) for causing browser-chrome failures in browser/components/newtab/test/browser/browser_aboutwelcome_multistage_experimentAPI.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/cd8dc51ea934a4747bba085e9dafc8b4aadccf27

Push with failures

Failure log

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb7262356afd
remove stub of enrollmentPromise from nimbus' ExperimentFakes r=barret

Comment 9

[Charlie Humphreys [:charlie] [:jeddai]]

Also missed the test failing in the previous backout. Fixed now and in the landing attempt above!

Comment 10

[Cristian Tuns]

Backed out for causing mochitest failures on browser_aboutwelcome_multistage_experimentAPI.js

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | browser/components/newtab/test/browser/browser_aboutwelcome_multistage_experimentAPI.js | Uncaught exception in test - Should render div.onboardingContainer in multistage step 1 - timed out after 50 tries.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:charlie, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 12

[Charlie Humphreys [:charlie] [:jeddai]]

I've paused work on this for a short time but will be coming back to it. I marked the patch as "Changes Planned" to note that there are still some outlying issues.

Comment 13

[Barret Rennie [:barret] (they/them)]

We can leave the stub as long as we replace it with a sync version.

Comment 14

[Barret Rennie [:barret] (they/them)]

Attachment: Bug 1773583 - Synchronously fetch NimbusEnrollment schema in NimbusTestUtils r?emcminn

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3642698fa9e4
Synchronously fetch NimbusEnrollment schema in NimbusTestUtils r=emcminn

Comment 16

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/3642698fa9e4