Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨Add AMP layer to E2E tests #20502

Merged
merged 7 commits into from Jan 28, 2019
Merged

Conversation

cvializ
Copy link
Contributor

@cvializ cvializ commented Jan 24, 2019

Related to #20457

This PR also reduces the height of the windows in some tests, since not all screens are large enough to allow windows to resize to be 1000px.

@estherkim and I discussed configuring experiments using the top-level configuration object, like the integration tests currently do, e.g.

describes.e2e('amp-blah', {experiments: ['amp-blah-experiment']}, () => {
  /*...*/
});

We will work toward that. That code will consume the AmpDriver instance and we can factor out the toggleExperiment calls at that time.

@estherkim
Copy link
Collaborator

Would it be too much to use TestSpec in describes-e2e for this PR (top level config as you mentioned)? Then you'd only need 1 single toggleExperiment call in setting up AmpPageFixture and it'd be cleaner from the get-go.

@estherkim
Copy link
Collaborator

Do we need a separate AmpDriver file? My understanding is that it's only used to set up AmpPageFixture. It would be a somewhat straight forward change if we move AmpDriver.toggleExperiment() into describes-e2e if we use TestSpec for test config since it's already carried through in the constructor.

@cvializ
Copy link
Contributor Author

cvializ commented Jan 24, 2019

I think there will be other use-cases for AMP-layer concepts in future tests, so to me it makes sense to have a separation of concerns where there is a specific AmpDriver class that's responsible for reads and writes to AMP-related state setup, and the AmpPageFixture class is responsible for reading any configuration and preparing the drivers/browsers.

I haven't solved the problem of experiments needing to be set when the browser is on a page on the domain that the test code runs on. And that problem needs to be solved before moving the toggleExperiment call into describes-e2e. For example, tests can open pages on any domain, but describes-e2e doesn't have knowledge about the pages that will be loaded, and the experiment will only be set on the browser's current domain. I think to maintain incremental progress that we should solve that problem in future PRs.

* @override
*/
eval_(fn, unusedArgs) {
const args = Array.prototype.slice.call(arguments, 1);

Choose a reason for hiding this comment

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

A bit unclear here, [fn].concat(args) looks to be the same thing as arguments, so why not just apply executeScript with arguments directly?

If you do want to do this pattern (e.g. if typing is an issue), this would be better:

eval_(fn, ...args) {
  return this.driver.executeScript(fn, ...args);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to use arguments directly because I wanted to enforce the use of a function as the first argument, since I believe Selenium's executeScript also accepts a string. The spread operator works better, thanks

* @return {!ControllerPromise}
* @override
*/
eval_(fn, unusedArgs) {

Choose a reason for hiding this comment

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

The style guide says trailing _ for @private methods, fields, etc. but does not say to use them for @package. Should remove the _.

* @param {!../functional-test-controller.FunctionalTestController} controller
*/
constructor(controller) {
this.controller_ = controller;

Choose a reason for hiding this comment

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

@private?

@cvializ
Copy link
Contributor Author

cvializ commented Jan 24, 2019

It'd be nice if we could run Closure Compiler to actually enforce the type information on these methods, but since these aren't really compile targets I'm not sure how we'd do that.

@cvializ cvializ merged commit 2e67e4c into ampproject:master Jan 28, 2019
jsalgueiro added a commit to jsalgueiro/amphtml that referenced this pull request Jan 29, 2019
* commit '273e1fe2ff7719bf555c9614afe19e14231e4dae':
  Changes to make carousel v2 work with lightbox gallery. (ampproject#20558)
  ✅ Additional carousel v2 end to end tests (ampproject#20522)
  ✨Get placeholder background from `placeholder` srcs (ampproject#20563)
  🐛Reparent placeholder to account for incorrect positioning (ampproject#20562)
  Fix amp-sidebar keyboard event forwarding (ampproject#20557)
  📖 Update `amp-video-docking` CSS reference (ampproject#20571)
  ✅Validate `dock` for `amp-youtube` (ampproject#20554)
  ✅Validate `dock` for `amp-brightcove` (ampproject#20567)
  ✅Add overflow prevention test (ampproject#20546)
  ✅Fix amp-date-display flakes (ampproject#20568)
  temporarily disable saucelabs because of flakiness (ampproject#20566)
  Fix closing the page attachment through popping the history state. (ampproject#20443)
  ✨Add AMP layer to E2E tests (ampproject#20502)
  add avenues for CLA help (ampproject#20521)
  provide more details on design reviews (ampproject#20533)
  🗑️ Remove `video-dock` experiment guard. (ampproject#20413)
  ♻️ amp-recaptcha-input: Allow passing the recaptcha API url from the frame (ampproject#20539)
  🐛 Fix amp-list documentation (ampproject#20553)
  launch amp-list-viewport-resize to 75% in prod (ampproject#20552)
@cvializ cvializ deleted the feature/e2e-experiment branch February 7, 2019 18:48
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
* Add AmpDriver for toggleExperiment. Enable cli debug flags

* Fix rebase munge

* Add AmpDriver to new tests and mocha setup

* Review feedback

* Use the document under test to set the experiment vs the root page

* Use spread argument

* Fix eval lint
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.

None yet

5 participants