994727 - Telemetry experiments: Test and fix how disabling the feature works.

Status: VERIFIED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Georg Fritzsche [:gfritzsche]]

We need to add test coverage of disabling the experiments feature.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Attachment: Test coverage for turning the experiments feature off

Comment 2

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix turning the feature off

This fixes the path for turning the feature off during a session.
This breaks down _evaluateExperiments() into more manageable chunks, i hope that shows up sanely via the diff.

Comment 3

[Georg Fritzsche [:gfritzsche]]

https://tbpl.mozilla.org/?tree=Try&rev=157d4dfeb83d

Comment 4

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix turning the feature off

Functionally equivalent but without the conflict-prone refactoring.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix turning the feature off

Comment 6

[Benjamin Smedberg]

Comment on attachment 8404836 [details] [diff] [review]
Fix turning the feature off

I don't see where this disabled the currently-running experiment. I'd expect that to be in _shouldStop?

Comment 7

[Georg Fritzsche [:gfritzsche]]

The currently running experiments is disabled via:
* _toggleExperimentsEnabled(false)
* disableExperiment() -> sets _terminateReason
* _main()
* _evaluateExperiments() -> picks up _terminateReason

This already worked fine, this patch assures we don't:
* re-activate the current experiment
* activate the next applicable experiment
* trigger manifest updates

Comment 8

[Benjamin Smedberg]

That covers most of the cases, indeed. Is there a reason we shouldn't also add a check in maybeStop? I'm a little worried about a case where we need to disable the experiment system in an update and there is an active experiment: we need to make sure that experiment is properly stopped.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix turning the feature off

Between restarts we would be covered by the fact that the experiment addons will always be disabled on startup until we explicitly enable them.
But a check in maybeStop() for robustness can't hurt either.

Comment 10

[Benjamin Smedberg]

Comment on attachment 8405341 [details] [diff] [review]
Fix turning the feature off

In maybeStop don't we still need to call this.stop() instead of just resolving the promise?

Comment 11

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix turning the feature off

Now with some coffee support on my side.

Comment 12

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix turning the feature off

Comment 13

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/fx-team/rev/2462ea3fd3c7
https://hg.mozilla.org/integration/fx-team/rev/e0b01e4f5fc3

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2462ea3fd3c7
https://hg.mozilla.org/mozilla-central/rev/e0b01e4f5fc3

Comment 15

[Georg Fritzsche [:gfritzsche]]

... not sure if it makes sense to track this after the fact (for the team stats etc.).

Comment 16

[Georg Fritzsche [:gfritzsche]]

I still don't have a good feel on matching point values to work items, guessing on a 3 here.

Comment 17

[Marco Mucci [:MarcoM]]

Hi Liz, can you review this bug to determine if it requires further QA verification [qa+] or not [qa-].

Comment 18

[Marco Mucci [:MarcoM]]

Hi Kamil, confirming if this resolved bug can be verified before the end of the desktop iteration on Monday April 28?

Comment 19

[Kamil Jozwiak [:kjozwiak]]

Went through verification with the latest Nightly build from the following location:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-04-29-03-02-01-mozilla-central/

- Disabled experiments via "experiments.enabled" in about:config and ensured that experiment wasn't being installed/initiated
- While experiments where disabled via about:config, ensured FX wasn't requesting firefox-manifest.json from the local staging server
- Ensured that "experiments.enabled" stayed disabled when restarting FX several times
- Ensured that once the experiment has been "Removed", re-loading about:addons and about:support didn't request the firefox-manifest from the local staging server
- Ensured that once the experiment has been "Removed", re-loading about:addons and about:support didn't re-enable the experiment
- Ensured that double clicking on a disabled experiment wasn't contact the local staging server and requesting firefox-manifest.json file
- Ensured that FX requests firefox-manifest from the local staging server when the experiment is disabled using "experiments.manifest.fetchIntervalSeconds"

I couldn't go through the test cases that ensure that the experiment stayed disabled after restarting FX due to Bug # 1001787. I'm going to mark this as verified and those cases that haven't been tested here will be covered in that bug once it's completed and pushed into Nightly. If someone disagree's, please re-open :)