Refactor the computation of experiment config. by lannka · Pull Request #6709 · ampproject/amphtml

lannka · 2016-12-16T00:28:04Z

This is a preparation work for #6684

dvoytenko · 2016-12-17T05:38:04Z

src/experiments.js

+    let enabled = true;
+    if (experimentId[0] == '-') {
+      // It is a  disabling flag.
+      experimentId = experimentId.substr(1);


Disable flag here takes precedence over enabled. This doesn't seem to be currently the case.

In fact, currently there is a bug that can be reproduced by:

AMP.toggleExperiment('-some-experiment', true); AMP.toggleExperiment('some-experiment', true); AMP.toggleExperiment('some-experiment', true);

This will give you duplicated entries in the cookie

That's to say, we should probably not have both "some-experiment" and "-some-experiment" in the cookie.

Possible. But the goal is very straightforward and I believe needed. We need to be able to explicitly disable an experiment that has already been launched. Does this allow us to do that?

Yes, we still have this feature. You can still disable an experiment by

AMP.toggleExperiment('-some-experiment', true);

My logic will allow you to re-enable it by

AMP.toggleExperiment('some-experiment', true);

The bug I was talking about is a separate issue we should fix.

After merging with the other refactoring, this is obsolete now. PTAL

dvoytenko · 2016-12-17T05:38:43Z

test/functional/test-experiments.js

-    it('should return "off" when disabling value is in the list', () => {
-      expectExperiment('AMP_EXP=-e1,e1', 'e1').to.be.false;
+    it('should return "off" when disabling value is after enabling', () => {
+      expectExperiment('AMP_EXP=-e1,e1', 'e1').to.be.true;


Hence here it should be false. Why are you switching it to true?

jridgewell · 2016-12-20T22:52:56Z

Does win._experimentCookie need to be exposed?

lannka · 2016-12-20T23:10:12Z

nope. we can hide it.

actually, shall we just remove this cache? it's only used by toggleExperiment() function, so i guess there should not be a performance concern?

dvoytenko · 2016-12-20T23:55:46Z

Let's remove

lannka · 2016-12-21T19:20:32Z

Removed. PTAL

dvoytenko

Couple requests, but otherwise LGTM

dvoytenko · 2016-12-21T19:29:56Z

src/experiments.js

+  if (win.AMP_CONFIG) {
+    for (const experimentId in win.AMP_CONFIG) {
+      if (win.AMP_CONFIG.hasOwnProperty(experimentId)) {
+        const frequency = win.AMP_CONFIG[experimentId];


We probably should use Object.prototype.hasOwnProperty.call, otherwise I don't think we can guarantee that it has prototype.

dvoytenko · 2016-12-21T19:31:01Z

test/functional/test-experiments.js


  afterEach(() => {
-    resetExperimentToggles_();
+    resetExperimentTogglesForTesting();


I'd kind of prefer we call this method in the _init_tests.js's afterEach. Could you please at least try? It might cause too many errors, but I think it should be ok.

lannka · 2016-12-21T22:18:31Z

@tdrl hi Terran, this PR made changes to traffic-experiments.js. would you take a look see if it makes sense to you?
The idea is we should not access AMP_CONFIG directly to toggle an experiment. Instead, we should always do it via toggleExperiment() function.

tdrl

Only substantial issue is that it looks like you've changed the semantics of a number of tests. Other than that, seems like a good cleanup to me.

tdrl · 2016-12-22T18:07:35Z

src/experiments.js

+  // Read the default config of this build.
+  if (win.AMP_CONFIG) {
+    for (const experimentId in win.AMP_CONFIG) {
+      const frequency = win.AMP_CONFIG[experimentId];


Do you need to check hasOwnProperty?

Seems not, I added a new test case to make sure.

tdrl · 2016-12-22T18:31:28Z

ads/google/a4a/traffic-experiments.js

-  if (getMode(win).localDev) {
-    // In local dev mode, it can be difficult to configure AMP_CONFIG
-    // externally.  Default it here if necessary.
-    win.AMP_CONFIG = win.AMP_CONFIG || {};


I assume this "AMP_CONFIG in local dev mode" initialization thing is now handled centrally by your refactor? I can't immediately find it.

As explained below, we let experiments.js to handle all the experiment status read write. No one should access AMP_CONFIG directly. How we store the status in AMP_CONFIG is really an implementation detail, and subject to change.

All the experiment flags are calculated once, no matter AMP_CONFIG exists or not.
https://github.com/ampproject/amphtml/blob/master/src/experiments.js#L134

tdrl · 2016-12-22T18:35:23Z

ads/google/a4a/test/test-traffic-experiments.js

    it('handles empty experiments list', () => {
      // Opt out of experiment.
-      sandbox.win.AMP_CONFIG['testExperimentId'] = 0.0;
+      toggleExperiment(sandbox.win, 'testExperimentId', false, true);


This changes the semantics of the test, doesn't it? The previous semantic was, "When AMP_CONFIG says that this experiment has 0 probability, then randomly selection should not turn it on." Here, you're forcing it off via the toggle and then testing to see that it's still off.

Ditto for the other places where you replace the AMP_CONFIG with a toggle.

That's exactly the point of this refactoring. We should NOT directly access AMP_CONFIG to read/write experiment status. We should always to it via the methods exposed by experiments.js.

In your unit test here, you don't really care how the experiment is set. You only care if the experiment is on.

No, I agree that you shouldn't have to touch AMP_CONFIG. But this experiment does do more than just care whether the experiment is on. randomlySelectUnsetPageExperiments does what its name says: it rolls the dice for each requested experiment. The test is verifying that it sets the experiment value as expected given that the experiment probability is a certain value. In this change, you're setting the experiment value before calling randomlySelectUnsetPageExperiments. That method will look and say, "Oh, that experiment already has a value. I won't touch it." So you're no longer testing its behavior, which is really the point of this test.

If I read the code correctly, the randomness of randomlySelectUnsetPageExperiments is about "branches", which we still have tests covering that. On the other side, AMP_CONFIG['testExperimentId'] = 0.0 is just turning off the experiment, which should have been done via toggleExperiment. Otherwise, it's just testing the logic inside toggleExperiment method.

I could have mis-read the logic here, and that's why to have you to help :-).

…us map.

lannka · 2016-12-22T20:41:01Z

ads/google/a4a/test/test-traffic-experiments.js

-      config['fooExpt'] = 0.0;
+      toggleExperiment(sandbox.win, 'fooExpt', false, true);
      randomlySelectUnsetPageExperiments(sandbox.win, exptAInfo);
-      config['fooExpt'] = 1.0;


I will remove this test case if no objection. Since the goal of this PR is to guarantee an immutable AMP_CONFIG (no one should change it), there's no point to have such a test.
@tdrl

lannka · 2016-12-22T23:05:23Z

@tdrl and I didn't reach a consensus on my changes in test-traffic-experiments.js, hence I reverted the changes and left a TODO for removing the direct access to AMP_CONFIG.

tdrl · 2016-12-22T23:22:13Z

LGTM.

* Refactor the computation of experiment config. * Removed cache for cookie toggles since it's only retrieved once. * Fix type checks * Fix tests * Remove unnecessary change of AMP_CONFIG for local testing. * Call resetExperimentToggleForTesting in _init_tests.js * Remove directly access to AMP_CONFIG for toggling experiments. * Add one more test case to make sure we have the right experiment status map. * Revert changes in test-traffic-experiments.js

Refactor the computation of experiment config.

52395ba

lannka assigned erwinmombay and dvoytenko Dec 16, 2016

dvoytenko suggested changes Dec 17, 2016

View reviewed changes

lannka mentioned this pull request Dec 20, 2016

Store either "someexp" or "-someexp" in cookie, never both. #6762

Merged

Merge master

57f7435

lannka added 2 commits December 20, 2016 15:22

Removed cache for cookie toggles since it's only retrieved once.

ccb2b30

Fix type checks

62cf5ff

dvoytenko approved these changes Dec 21, 2016

View reviewed changes

lannka added 4 commits December 21, 2016 13:26

Fix tests

e7ca57f

Remove unnecessary change of AMP_CONFIG for local testing.

91dc915

Call resetExperimentToggleForTesting in _init_tests.js

341e728

Remove directly access to AMP_CONFIG for toggling experiments.

ae39880

lannka assigned tdrl Dec 21, 2016

tdrl reviewed Dec 22, 2016

View reviewed changes

Add one more test case to make sure we have the right experiment stat…

ae3ad11

…us map.

lannka commented Dec 22, 2016

View reviewed changes

Revert changes in test-traffic-experiments.js

f2c2396

lannka merged commit 78bdf43 into ampproject:master Dec 22, 2016

lannka deleted the experiment-toggle branch December 22, 2016 23:23

lannka mentioned this pull request Dec 22, 2016

Remove direct access to AMP_CONFIG in test-traffic-experiments.js #6799

Merged

Conversation

lannka commented Dec 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jridgewell commented Dec 20, 2016

Uh oh!

lannka commented Dec 20, 2016

Uh oh!

dvoytenko commented Dec 20, 2016

Uh oh!

lannka commented Dec 21, 2016

Uh oh!

dvoytenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lannka commented Dec 21, 2016

Uh oh!

tdrl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lannka commented Dec 22, 2016

Uh oh!

tdrl commented Dec 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants