✨Integrate E2E Testing with demo viewer and demo PWA shadow doc. #20994

cvializ · 2019-02-21T20:54:16Z

Resolves #20575

This is a work in progress PR for integrating the various environments with end-to-end testing. This also introduces a new fixture format. It currently only runs the test-carousel.js tests using the new format, and breaks the others for now.

Feel free to leave some feedback

describes.endtoend('AMP carousel', { 
  fixture: 'http://localhost:8000/test/manual/amp-carousel-0-2/basic.amp.html',
  experiments: ['amp-carousel-v2'], 
  environment: 'viewer-demo', // or 'single' or 'shadow-demo'
}, async env => {
  /* ... */
});

sparhami · 2019-02-22T00:11:09Z

build-system/tasks/e2e/driver/query-xpath.js

+    node.dataset['iAmphtmlTestId'] = value;
+  };
+  const removeData = node => delete node.dataset['iAmphtmlTestId'];
+  const getIt =


I think it might be cleaner to have an iterator that gives you what you want, and have your iteration code be straightforward:

function* createIterator(context) { const it = document.createNodeIterator(context, NodeFilter.SHOW_ELEMENT); let node; let index = 0; while(node = it.nextNode()) { index++; yield { node, index }; } }

and use it as:

for (const {node, index} of createIterator(context)) { toggleData(node, index); }

I would consider a step further and compose:

function* createSimpleElementIterator(context) { const it = document.createNodeIterator(context, NodeFilter.SHOW_ELEMENT); while(node = it.nextNode()) { yield node; } } function* createIndexedInterator(iter) { let index = 0; for (const item of iter) { index++; yield {item, index}; } } const iter = createIndexedIterator(createSimpleElementIterator(context));

because now you will have generic composable building blocks that can ideally live outside your code.

Good idea, now that we're using async/await it makes sense to use generators as well.

sparhami · 2019-02-22T01:40:46Z

build-system/tasks/e2e/selenium-webdriver-controller.js

-    const webElement = await this.driver.findElement(byXpath);
+    await this.maybeInstallXpath_();
+
+    const webElement = await this.driver.wait(new Condition('', async() => {


sparhami · 2019-02-22T01:43:07Z

build-system/tasks/e2e/selenium-webdriver-controller.js

+    }
+    this.isXpathInstalled_ = true;
+
+    const script = fs.readFileSync('third_party/wgxpath/wgxpath.js', 'utf8');


I'm pretty sure that fs.readFileAsync returns a Promise, so would be better to do that rather than have a sync call here when everything else is async.

Nice, TIL. I thought there were only continuation passing readFile and the sync readFileSync

Looks like there is no readFileAsync, but there is an experimental feature fsPromises:

const {readFile} = require('fs').promises;

Ah, it looks like we have this in our build-system:

const BBPromise = require('bluebird'); const fs = BBPromise.promisifyAll(require('fs'));

which is what is adding the async versions. Could use that to stay consistent.

Huh neat, thanks

sparhami · 2019-02-22T01:59:43Z

extensions/amp-carousel/0.2/test-e2e/test-carousel.js

@@ -21,32 +21,27 @@ import {
 } from './helpers';

 describes.endtoend('AMP carousel', {
+  fixture: 'http://localhost:8000/test/manual/amp-carousel-0-2/basic.amp.html',
+  experiments: ['amp-carousel-v2'],
+  environment: 'shadow-demo',


Is the plan that if you want to test under multiple environments, you create multiple describes sharing the same test runner function?

Yes, or a describes.repeated. It might be possible to pass an array and do the describes.repeated under the hood, but I'm not sure how I'd work that right now

IMO one of the most important features for e2e testing is to automatically run a given test on all environments by default. Happy to chat about what this would require.

/cc @rsimha

I'd imagine we can use a describes.repeated block to iterate across the different types of Environments in AmpEnvironments, just like this code goes over the variants for templateType.

amphtml/extensions/amp-mustache/0.1/test/test-amp-mustache.js

Lines 22 to 72 in ce617b7

describes.repeated('amp-mustache 0.1', {

'with script[type=text/plain][template=amp-mustache]':

{templateType: 'script'},

'with template[type=amp-mustache]':

{templateType: 'template'},

}, (name, variant) => {

let sandbox;

let viewerCanRenderTemplates = false;

beforeEach(() => {

sandbox = sinon.sandbox;

const getServiceForDocStub = sandbox.stub(service, 'getServiceForDoc');

getServiceForDocStub.returns({

hasCapability: unused => viewerCanRenderTemplates,

});

});

afterEach(() => sandbox.restore());

let innerHtmlSetup;

let template;

let templateElement;

let textContentSetup;

let isTemplateType;

let isTemplateTypeScript;

beforeEach(() => {

const {templateType} = variant;

templateElement = document.createElement(templateType);

if (templateType == 'script') {

templateElement.setAttribute('type', 'amp-mustache');

}

template = new AmpMustache(templateElement);

isTemplateTypeScript = templateType == 'script';

isTemplateType = templateType == 'template';

textContentSetup = contents => {

if (isTemplateType) {

templateElement.content.textContent = contents;

} else if (isTemplateTypeScript) {

templateElement.textContent = contents;

}

};

innerHtmlSetup = html => {

if (isTemplateType) {

templateElement./*OK*/innerHTML = html;

} else if (isTemplateTypeScript) {

templateElement.textContent = html;

}

};

});

/cc @alabiaga

@rsimha that's what I was imagining as well. This would be the easiest implementation for now. We can refactor later to allow passing an array and have the describes.e2e handle the repetition, if that is ok with everyone

SGTM. I like this because it allows us to define a pattern in once place and reuse it across several tests.

/cc @estherkim

examples/viewer.html

build-system/tasks/e2e/describes-e2e.js

estherkim · 2019-02-25T15:34:29Z

build-system/tasks/e2e/selenium-webdriver-controller.js

+    const webElement = await this.driver.wait(new Condition('', async() => {
+      try {
+        const root = await this.getRoot_();
+        const results = await this.evaluate(queryXpath, xpath, root);


What about using selenium's built-in locator? https://seleniumhq.github.io/selenium/docs/api/javascript/module/selenium-webdriver/index_exports_By.html#By.xpath

The built-in XPath locator doesn't work for elements inside ShadowRoots, unfortunately. The wgxpath library added in third_party allows evaluating XPath queries in detached document trees as well. The comment block in query-xpath.js has a detailed description. Let me know if I can help with any questions about that

examples/viewer.html

estherkim · 2019-02-25T15:48:42Z

Newbie question about examples/viewer.html - is the purpose of this page to mimic the viewer in production? If so, and the viewer changes, how do we know to keep it up to date?

cvializ · 2019-02-25T15:56:26Z

I think you're right. The page implements some of the viewer API, but I don't think it implements all of it. As I understand it, this is meant to run the core viewer API like for loading documents in an iframe, but it doesn't intend to implement every feature like server side template rendering. /cc @alabiaga or @choumx to check my understanding

dreamofabear · 2019-02-25T16:19:48Z

extensions/amp-carousel/0.2/test-e2e/test-carousel.js

@@ -21,32 +21,27 @@ import {
 } from './helpers';

 describes.endtoend('AMP carousel', {
+  fixture: 'http://localhost:8000/test/manual/amp-carousel-0-2/basic.amp.html',
+  experiments: ['amp-carousel-v2'],
+  environment: 'shadow-demo',


IMO one of the most important features for e2e testing is to automatically run a given test on all environments by default. Happy to chat about what this would require.

/cc @rsimha

dreamofabear · 2019-02-25T16:22:40Z

extensions/amp-carousel/0.2/test-e2e/test-carousel.js

@@ -21,32 +21,27 @@ import {
 } from './helpers';

 describes.endtoend('AMP carousel', {
+  fixture: 'http://localhost:8000/test/manual/amp-carousel-0-2/basic.amp.html',


One of the nice things about describes.integration is that it abstracts away boilerplate markup (e.g. <head> content) and inlines the test fixture. This also makes it easy to reuse a fixture/test tuple in different validator specs. Check out #19943.

I think we should also do that here if possible.

alabiaga · 2019-02-25T16:59:06Z

Server side template rendering is tested in this example viewer (capability is called viewerRenderTemplate) but note that the actual proxying of the request from the viewer's processRequest_ method to an actual REST end point doesn't happen. I just mock the rendered response expected from the server. So it isn't a true end to end test but should suffice as long email clients conform to the expected JSON format response required.

But yes this viewer also doesn't fully test the other capabilities that viewers can support.

build-system/tasks/e2e/describes-e2e.js

build-system/tasks/e2e/driver/query-xpath.js

build-system/tasks/e2e/selenium-webdriver-controller.js

alabiaga · 2019-03-01T14:01:43Z

build-system/tasks/e2e/amp-driver.js

+
+/** @enum {string} */
+const Environment = {
+  SINGLE: 'single',


I'm not sure how I feel about this name. We mean single doc yes, SINGLE_DOC?

Hmm, there's a precedent for single as an ampdoc environment.

amphtml/extensions/amp-ad/0.1/test/test-amp-ad-custom.js

Lines 141 to 145 in e194f8e

describes.realWin('with single AmpDoc', {

amp: {

ampdoc: 'single',

},

}, env => {

Does the precedent make you feel better about it?

Gotcha, but here the value single is associated with the property ampdoc and so it reads well. In your case you're associating single with an environment, which is not intuitive to me. Though the following would be clearer.

const AMP_DOC_ENVIRONMENT = { SINGLE: 'single', }

then it would be clearer. I don't feel strongly about this. Perhaps the fact that others have not pointed this out probably shows that I am the only one not too keen about the naming.

alabiaga · 2019-03-01T14:02:19Z

build-system/tasks/e2e/amp-driver.js

+  /**
+   * Navigate the browser to a URL that will display the given url in the
+   * given environment.
+   * @param {Environment} environment


non null !Environment.

alabiaga · 2019-03-01T14:06:07Z

build-system/tasks/e2e/amp-driver.js

+   */
+  async serveMode(mode) {
+    await this.controller_.navigateTo(
+        `http://localhost:8000/serve_mode=${mode}`);


I am assuming host, port and protocol here will eventually be configurable and not hardcoded here and everywhere else, as the demo server allows.

It will be eventually, but for this PR I'd say it's out of scope. I'll add a TODO

alabiaga · 2019-03-01T14:34:02Z

build-system/tasks/e2e/amp-driver.js

+  SHADOW_DEMO: 'shadow-demo',
+};
+
+const AmpEnvironments = {


nitpick. Perhaps just Environments as it sort of is implied that it's related to amp? and Environment constant should be EnvironmentType?

cvializ · 2019-03-04T18:14:03Z

Thanks for the review. I'm going to split this into smaller PRs

cvializ requested review from alabiaga, sparhami and estherkim February 21, 2019 20:54

googlebot added the cla: yes label Feb 21, 2019

sparhami reviewed Feb 22, 2019

View reviewed changes

examples/viewer.html Outdated Show resolved Hide resolved

estherkim reviewed Feb 25, 2019

View reviewed changes

dreamofabear reviewed Feb 25, 2019

View reviewed changes

cvializ added 8 commits February 27, 2019 16:46

Fix race in the viewer where doc doesn't receive visible

f7a3ffb

Add support for viewer testing to e2e

53a9d36

Add support for testing shadowdom environment

42d77de

Cleanup and fixes

22c37aa

This breaks shadowdom but needs to be uncommented

7e6bda3

Cleanup

f7a13bf

use generator syntax

0522d61

Fix for new amp-base-carousel

e797dba

cvializ force-pushed the e2e/viewer branch from d4b3b74 to e797dba Compare February 27, 2019 22:28

cvializ added 2 commits February 28, 2019 11:04

Use same hashparam that the pwa demo does

95ab9e5

Iterate through test environments by default. Cleanup

f454a71

sparhami suggested changes Feb 28, 2019

View reviewed changes

Rethrow if not explicitly a NoSuchElementError

debe027

cvializ force-pushed the e2e/viewer branch from 111289a to aa3391d Compare February 28, 2019 21:13

Formatting

e194f8e

cvializ force-pushed the e2e/viewer branch from aa3391d to e194f8e Compare February 28, 2019 22:08

alabiaga suggested changes Mar 1, 2019

View reviewed changes

More Feedback

3633f51

cvializ mentioned this pull request Mar 4, 2019

✨Integrate all e2e tests with viewer environment #21236

Merged

cvializ closed this Mar 4, 2019

cvializ mentioned this pull request Mar 6, 2019

✨Enable shadow-demo environment for e2e tests for documents inside PWAs #21298

Merged

cvializ deleted the e2e/viewer branch April 2, 2019 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨Integrate E2E Testing with demo viewer and demo PWA shadow doc. #20994

✨Integrate E2E Testing with demo viewer and demo PWA shadow doc. #20994

cvializ commented Feb 21, 2019 •

edited

sparhami Feb 22, 2019

cvializ Feb 22, 2019

sparhami Feb 22, 2019

sparhami Feb 22, 2019

cvializ Feb 22, 2019

cvializ Feb 22, 2019

sparhami Feb 22, 2019

cvializ Feb 22, 2019

sparhami Feb 22, 2019

cvializ Feb 22, 2019 •

edited

dreamofabear Feb 25, 2019

rsimha Feb 25, 2019 •

edited

cvializ Feb 25, 2019

rsimha Feb 25, 2019

estherkim Feb 25, 2019

cvializ Feb 25, 2019 •

edited

estherkim commented Feb 25, 2019

cvializ commented Feb 25, 2019 •

edited

dreamofabear Feb 25, 2019

dreamofabear Feb 25, 2019

alabiaga commented Feb 25, 2019

alabiaga Mar 1, 2019

cvializ Mar 1, 2019 •

edited

alabiaga Mar 1, 2019

alabiaga Mar 1, 2019

alabiaga Mar 1, 2019

cvializ Mar 1, 2019

alabiaga Mar 1, 2019

cvializ commented Mar 4, 2019

	describes.repeated('amp-mustache 0.1', {
	'with script[type=text/plain][template=amp-mustache]':
	{templateType: 'script'},
	'with template[type=amp-mustache]':
	{templateType: 'template'},
	}, (name, variant) => {

	let sandbox;
	let viewerCanRenderTemplates = false;

	beforeEach(() => {
	sandbox = sinon.sandbox;
	const getServiceForDocStub = sandbox.stub(service, 'getServiceForDoc');
	getServiceForDocStub.returns({
	hasCapability: unused => viewerCanRenderTemplates,
	});
	});

	afterEach(() => sandbox.restore());

	let innerHtmlSetup;
	let template;
	let templateElement;
	let textContentSetup;
	let isTemplateType;
	let isTemplateTypeScript;

	beforeEach(() => {
	const {templateType} = variant;
	templateElement = document.createElement(templateType);
	if (templateType == 'script') {
	templateElement.setAttribute('type', 'amp-mustache');
	}
	template = new AmpMustache(templateElement);
	isTemplateTypeScript = templateType == 'script';
	isTemplateType = templateType == 'template';
	textContentSetup = contents => {
	if (isTemplateType) {
	templateElement.content.textContent = contents;
	} else if (isTemplateTypeScript) {
	templateElement.textContent = contents;
	}
	};
	innerHtmlSetup = html => {
	if (isTemplateType) {
	templateElement./OK/innerHTML = html;
	} else if (isTemplateTypeScript) {
	templateElement.textContent = html;
	}
	};
	});

	describes.realWin('with single AmpDoc', {
	amp: {
	ampdoc: 'single',
	},
	}, env => {

✨Integrate E2E Testing with demo viewer and demo PWA shadow doc. #20994

✨Integrate E2E Testing with demo viewer and demo PWA shadow doc. #20994

Conversation

cvializ commented Feb 21, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvializ Feb 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsimha Feb 25, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvializ Feb 25, 2019 • edited

Choose a reason for hiding this comment

estherkim commented Feb 25, 2019

cvializ commented Feb 25, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alabiaga commented Feb 25, 2019

Choose a reason for hiding this comment

cvializ Mar 1, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvializ commented Mar 4, 2019

cvializ commented Feb 21, 2019 •

edited

cvializ Feb 22, 2019 •

edited

rsimha Feb 25, 2019 •

edited

cvializ Feb 25, 2019 •

edited

cvializ commented Feb 25, 2019 •

edited

cvializ Mar 1, 2019 •

edited