Skip to content

Conversation

calebcordry
Copy link
Member

@calebcordry calebcordry commented Jun 3, 2020

Adds diversion point, experiment will not be launched until logic is complete.

Part of #27189

@amp-owners-bot
Copy link

amp-owners-bot bot commented Jun 3, 2020

Hey @Jiaming-X! These files were changed:

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

Hey @jeffkaufman! These files were changed:

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js
extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

this.startValidationFlow_(fetchResponse, checkStillCurrent)
getExperimentBranch(this.win, NO_SIGNING_EXP.id) ===
NO_SIGNING_EXP.control
? this.streamResponse_(fetchResponse)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be the reverse?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes, good catch. No idea why the tests passed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I think its because this returns undefined

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ye, actually should change control to experiment, as @powerivq pointed out in the other comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

.then((fetchResponse) =>
this.startValidationFlow_(fetchResponse, checkStillCurrent)
getExperimentBranch(this.win, NO_SIGNING_EXP.id) ===
NO_SIGNING_EXP.control
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NO_SIGNING_EXP.experiment here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, done.

@calebcordry calebcordry merged commit d99658a into ampproject:master Jun 4, 2020
@calebcordry calebcordry deleted the a4a-sign-exp branch June 4, 2020 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants