A4A integration tests #5812

tdrl · 2016-10-25T19:13:39Z

First cut at A4A integration tests: Test the complete A4A flow, in AMP context, without calling out to a real ad server. Test set is not as complete as we'd like yet, but provides a template for further tests.

tdrl · 2016-10-25T19:14:38Z

/cc @ampproject/a4a

tdrl · 2016-10-27T20:10:49Z

Ugh. Travis is failing with a pile of Error: Cannot build unupgraded element errors. Of course, it works on my machine. Possibly a Chrome version difference? (Travis 51.0.2704 vs my machine 54.0.2840). Still diagnosing.

cramforce · 2016-10-27T22:01:30Z

Maybe rebase? @aghassemi just switched travis to use current stable Chrome.

lannka · 2016-10-28T02:34:45Z

extensions/amp-a4a/0.1/test/test-a4a-integration.js

+  });
+});
+
+describe('integration test: a4a', () => {


pls take a look at describes.js. It provides iframe isolation, should save a lot boilerplate code here.

Nifty. I wasn't aware of those -- thanks for pointing them out!

In this specific case, though, I'm afraid I don't see how to apply them to save a lot of boilerplate. I looked at doing:

describes.sandboxed('integration test: a4a', {}, () => { describes.realWin('another name', {amp: true}, env => { } }

for the setup. But then I still have to do a bunch of test-specific setup: mocking the XHR, setting up my expected response, building the amp-a4a element, calling upgradeOrRegisterElement to bind the mock impl to amp-a4a, etc. That's the majority of the setup space, so I don't save a lot. Plus, I lose the nice addElement method that createIframePromise provides, so I have to do some lifecycle stuff myself in the individual tests.

Unless I'm misunderstanding how to use those test fixtures? If so, I'm afraid you'll have to spell out a more explicit example.

I'm fine to leave it as is. In general, addElement is OK for a "integration" test like this one, but we'd like to avoid for unit test (no need to initialize the AMP runtime).

Ok, good to know. Yeah, for these tests, we really do want the AMP runtime in action. We really want to test that the interaction between A4A and AMP is correct / as expected. But it's a good thing to know for other, more unit-level tests. Thanks for the pointer.

lannka · 2016-10-28T02:36:34Z

extensions/amp-a4a/0.1/test/test-a4a-integration.js

+  });
+});
+
+chai.Assertion.addMethod('renderedInXDomainIframe', function(src) {


maybe just make it a local function? this add a method to global chai, it's the same effect as you add it in _init_tests.js

Ugh. Didn't realize that would have a side-effect beyond this file. What a pain. Updated to local functions.

lannka · 2016-10-28T02:40:17Z

extensions/amp-a4a/0.1/test/utils.js

+}
+
+export function stringToArrayBuffer(str) {
+  return new TextEncoder('utf-8').encode(str);


this will fail in browsers that don't support TextEncoder.
you can actually just use utils/bytes.js

Oh, cool. Done.

lannka · 2016-10-28T02:49:18Z

Error: Cannot build unupgraded element

did you run tests locally with flag --files=? If so, remove the flag and use describe.only() to only run your tests. that will give you the same file import as in Travis.

i met the exact error msg before. Not sure if it is the case here, but it was because i left an amp-ad element in the DOM, which has triggered the custom element build. I fixed it by renaming it to a different tag name, as my test wasn't really to test amp-ad behavior.

tdrl · 2016-11-01T17:55:30Z

PTAL

lannka

some more minor comments

lannka · 2016-11-02T20:14:52Z

extensions/amp-a4a/0.1/test/test-a4a-integration.js

+  expect(child.getAttribute('srcdoc')).to.contain.string(srcdoc);
+  const childBody = child.contentDocument.body;
+  expect(childBody, 'body of iframe doc').to.be.ok;
+  [element, child, childBody].forEach(toTest => {


nitpick: i would avoid the loop here, state them one by one potentially gives you more info when it fails (line number tells you which element is not visible).

Fair enough. I wrote the loop when I was doing more stuff individually to check visibility, but you're right that it's not as helpful now. Updated. (Also added failure messages for additional clarification.)

lannka · 2016-11-02T20:14:57Z

extensions/amp-a4a/0.1/test/test-a4a-integration.js

+  const child = element.querySelector('iframe[src]');
+  expect(child, 'iframe child').to.be.ok;
+  expect(child.getAttribute('src')).to.contain.string(src);
+  [element, child].forEach(toTest => {


lannka · 2016-11-02T20:31:26Z

extensions/amp-a4a/0.1/test/test-a4a-integration.js

+      });
+});
+
+// TODO(@ampproject/a4a): Need a test that double-checks that thrown errors


FYI, you can leave an empty it as a test case to be implemented

// TODO: xxx it('should blabla');

Done. Though really this is a note to figure out how to integrate this into all tests. Added some clarification to the TODO.

lannka · 2016-11-02T20:36:53Z

extensions/amp-a4a/0.1/test/test-a4a-integration.js

+          signature: null,
+        })
+        .onSecondCall().throws(new Error(
+            'Testing extractCreativeAndSignature should not occur error'));


is this to make sure there's no second call to the function?
if so, it might be clearer to do

expect(functionStub).to.be.calledOnce;

tdrl · 2016-11-03T14:00:17Z

Thanks! PTAL.

lannka

LGTM.

…ff to utils.js.

…tion rewrite (ampproject#5982) * Update amp-ad-network-doubleclick-impl.md * Reverting doc changes The previous change was made in error (wrong branch). * Initial changes, need to update tests * Start fixing tests * Fix unit test coverage * revert a4a examples Introduce amp-auto-id attribute to AMP element. (ampproject#5973) * Introduce amp-auto-id attribute to AMP element. * Fix tests * Address comments * Address comments * revert amp-live-list.md polling clarification (ampproject#5991) Only report 1% of errors if a page has non-AMP JS (ampproject#5994) Fixes ampproject#5732 Allow for hosted testing to override where third party frame is retrieved from (ampproject#5890) * Allow for hosted testing to override where third party frame is retrieved from * test my own site * fix ava tests Remove unsupported query selector feature (ampproject#5999) Workaround for misbehaving webview viewer (ampproject#6001) A webview viewer from Google is seing origins of the form `www.google.com`. For the time being we accept these as they trigger errors in our error reporting that aren't useful. As this form of origins can only occur with webviews that have full control over the page via JS injection anyway, this change should make no difference for normal web usage. A4A integration tests (ampproject#5812) * First draft of full integration tests for A4A. * First draft of full integration tests for A4A. * Got tests of exception conditions working. Refactored some common stuff to utils.js. * Refactored XDom and friendly iframe tests to helpers. * Added tests. * Resolved merge conflicts. * Centralized visibility check and updated visibility test. * Moved all A4A tests to using central visibility test. * Lint fixes. * Changed custom expectations to local functions. * Replaced stringToArrayBuffer with utf8Encode. * Fixed visibility assertion. * Lint fixes. * Added a comment (partly in order to get Travis to re-run). * Changes in response to reviews. * Rebased and fixed small rebase error. Fixes an issue where ads are not correctly centered on certain platforms. (ampproject#6003) * Centralized creative centering in doubleclick.js, and changed css 'transform' to a more commonly supported property. * Added check for existence and changed to plain old 'transform'. * Now asserting the existence of the container. amp-sticky-ad close button new style (ampproject#5979) * unit style * fix css * fix test * rebase * fix z-index Other JS errors: Use startsWith (ampproject#6006) Fixes ampproject#5994 (comment). Fix log calls without TAGNAME (ampproject#6005) My presubmit was a bit too lenient, and too strict at the same time. refactor amp-ad.css (ampproject#5992) Combine amp-analytics var docs with var substitutions doc re: ampproject#1302 (ampproject#5576) * Combine amp-analytics var docs with var substitutions doc re: ampproject#1302 * Updated non-supported vars + format changes for amp-analytics vars * Updates per Avi's feedback * Updated descriptions for amp-carousel vars add version parameter to AMP.extension signature (ampproject#5989) * add version parameter to AMP.extension signature * apply recommendations * remove TODO Improve error reporting (ampproject#6019) - Include the current `location.hash`. We are missing it, because it is not included in the referrer. - Include previously recorded errors, so it is easier to identify follow on errors. - Kill the 2000 char limit as it isn't important for AMP's target browsers. Include the originalHash (ampproject#6020) We remove the hash in embedding mode, so the original code doesn't actually work. Fix error: "Can't find variable: TextDecoder" (ampproject#6011) * Update amp-ad-network-doubleclick-impl.md * Reverting doc changes The previous change was made in error (wrong branch). * Initial changes, need to update tests * Start fixing tests * Fix unit test coverage * revert a4a examples * Include self (window) when verifying TextDecoder/TextEncoder existence * fix upstream conflict * PR feedback * PR feedback

Update ads.amp.html Modify A4A AMP Creative to use ampRuntimeUtf16CharOffsets from validation rewrite (ampproject#5982) * Update amp-ad-network-doubleclick-impl.md * Reverting doc changes The previous change was made in error (wrong branch). * Initial changes, need to update tests * Start fixing tests * Fix unit test coverage * revert a4a examples Introduce amp-auto-id attribute to AMP element. (ampproject#5973) * Introduce amp-auto-id attribute to AMP element. * Fix tests * Address comments * Address comments * revert amp-live-list.md polling clarification (ampproject#5991) Only report 1% of errors if a page has non-AMP JS (ampproject#5994) Fixes ampproject#5732 Allow for hosted testing to override where third party frame is retrieved from (ampproject#5890) * Allow for hosted testing to override where third party frame is retrieved from * test my own site * fix ava tests Remove unsupported query selector feature (ampproject#5999) Workaround for misbehaving webview viewer (ampproject#6001) A webview viewer from Google is seing origins of the form `www.google.com`. For the time being we accept these as they trigger errors in our error reporting that aren't useful. As this form of origins can only occur with webviews that have full control over the page via JS injection anyway, this change should make no difference for normal web usage. A4A integration tests (ampproject#5812) * First draft of full integration tests for A4A. * First draft of full integration tests for A4A. * Got tests of exception conditions working. Refactored some common stuff to utils.js. * Refactored XDom and friendly iframe tests to helpers. * Added tests. * Resolved merge conflicts. * Centralized visibility check and updated visibility test. * Moved all A4A tests to using central visibility test. * Lint fixes. * Changed custom expectations to local functions. * Replaced stringToArrayBuffer with utf8Encode. * Fixed visibility assertion. * Lint fixes. * Added a comment (partly in order to get Travis to re-run). * Changes in response to reviews. * Rebased and fixed small rebase error. Fixes an issue where ads are not correctly centered on certain platforms. (ampproject#6003) * Centralized creative centering in doubleclick.js, and changed css 'transform' to a more commonly supported property. * Added check for existence and changed to plain old 'transform'. * Now asserting the existence of the container. amp-sticky-ad close button new style (ampproject#5979) * unit style * fix css * fix test * rebase * fix z-index Other JS errors: Use startsWith (ampproject#6006) Fixes ampproject#5994 (comment). Fix log calls without TAGNAME (ampproject#6005) My presubmit was a bit too lenient, and too strict at the same time. refactor amp-ad.css (ampproject#5992) Combine amp-analytics var docs with var substitutions doc re: ampproject#1302 (ampproject#5576) * Combine amp-analytics var docs with var substitutions doc re: ampproject#1302 * Updated non-supported vars + format changes for amp-analytics vars * Updates per Avi's feedback * Updated descriptions for amp-carousel vars add version parameter to AMP.extension signature (ampproject#5989) * add version parameter to AMP.extension signature * apply recommendations * remove TODO Improve error reporting (ampproject#6019) - Include the current `location.hash`. We are missing it, because it is not included in the referrer. - Include previously recorded errors, so it is easier to identify follow on errors. - Kill the 2000 char limit as it isn't important for AMP's target browsers. Include the originalHash (ampproject#6020) We remove the hash in embedding mode, so the original code doesn't actually work. Fix error: "Can't find variable: TextDecoder" (ampproject#6011) * Update amp-ad-network-doubleclick-impl.md * Reverting doc changes The previous change was made in error (wrong branch). * Initial changes, need to update tests * Start fixing tests * Fix unit test coverage * revert a4a examples * Include self (window) when verifying TextDecoder/TextEncoder existence * fix upstream conflict * PR feedback * PR feedback

* First draft of full integration tests for A4A. * First draft of full integration tests for A4A. * Got tests of exception conditions working. Refactored some common stuff to utils.js. * Refactored XDom and friendly iframe tests to helpers. * Added tests. * Resolved merge conflicts. * Centralized visibility check and updated visibility test. * Moved all A4A tests to using central visibility test. * Lint fixes. * Changed custom expectations to local functions. * Replaced stringToArrayBuffer with utf8Encode. * Fixed visibility assertion. * Lint fixes. * Added a comment (partly in order to get Travis to re-run). * Changes in response to reviews. * Rebased and fixed small rebase error.

tdrl added WG: monetization Type: Feature Request Related to: Testing labels Oct 25, 2016

tdrl assigned lannka Oct 25, 2016

lannka requested changes Oct 28, 2016

View reviewed changes

lannka approved these changes Nov 2, 2016

View reviewed changes

lannka approved these changes Nov 3, 2016

View reviewed changes

lannka added LGTM and removed NEEDS REVIEW labels Nov 3, 2016

Terran Lane added 14 commits November 3, 2016 13:44

First draft of full integration tests for A4A.

31a42ab

First draft of full integration tests for A4A.

e916def

Got tests of exception conditions working. Refactored some common stu…

9c5d96b

…ff to utils.js.

Refactored XDom and friendly iframe tests to helpers.

9a08b60

Added tests.

af29b01

Resolved merge conflicts.

0de8f8e

Centralized visibility check and updated visibility test.

481fe00

Moved all A4A tests to using central visibility test.

3051838

Lint fixes.

c6d3e55

Changed custom expectations to local functions.

4fd797c

Replaced stringToArrayBuffer with utf8Encode.

1848de6

Fixed visibility assertion.

59e9634

Lint fixes.

0c72a3c

Added a comment (partly in order to get Travis to re-run).

487d149

Terran Lane added 2 commits November 3, 2016 13:51

Changes in response to reviews.

fbda8c8

Rebased and fixed small rebase error.

b384663

lannka merged commit 46bd246 into ampproject:master Nov 3, 2016

tdrl deleted the a4a-integration-tests branch November 3, 2016 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A4A integration tests #5812

A4A integration tests #5812

tdrl commented Oct 25, 2016

tdrl commented Oct 25, 2016

tdrl commented Oct 27, 2016

cramforce commented Oct 27, 2016

lannka Oct 28, 2016

tdrl Nov 1, 2016

lannka Nov 2, 2016

tdrl Nov 3, 2016

lannka Oct 28, 2016

tdrl Nov 1, 2016

lannka Oct 28, 2016

tdrl Nov 1, 2016

lannka commented Oct 28, 2016

tdrl commented Nov 1, 2016

lannka left a comment

lannka Nov 2, 2016

tdrl Nov 3, 2016

lannka Nov 2, 2016

tdrl Nov 3, 2016

lannka Nov 2, 2016

tdrl Nov 3, 2016

lannka Nov 2, 2016

tdrl Nov 3, 2016

tdrl commented Nov 3, 2016

lannka left a comment

A4A integration tests #5812

A4A integration tests #5812

Conversation

tdrl commented Oct 25, 2016

tdrl commented Oct 25, 2016

tdrl commented Oct 27, 2016

cramforce commented Oct 27, 2016

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

lannka commented Oct 28, 2016

tdrl commented Nov 1, 2016

lannka left a 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

Choose a reason for hiding this comment

tdrl commented Nov 3, 2016

lannka left a comment

Choose a reason for hiding this comment