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.

Sign up for GitHub

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

A4A integration tests #5812

Merged
merged 16 commits into from
Nov 3, 2016
Merged

A4A integration tests #5812

Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
31a42ab
First draft of full integration tests for A4A.
Oct 25, 2016
e916def
First draft of full integration tests for A4A.
Oct 25, 2016
9c5d96b
Got tests of exception conditions working. Refactored some common st…
Oct 25, 2016
9a08b60
Refactored XDom and friendly iframe tests to helpers.
Oct 25, 2016
af29b01
Added tests.
Oct 25, 2016
0de8f8e
Resolved merge conflicts.
Oct 25, 2016
481fe00
Centralized visibility check and updated visibility test.
Oct 26, 2016
3051838
Moved all A4A tests to using central visibility test.
Oct 26, 2016
c6d3e55
Lint fixes.
Oct 27, 2016
4fd797c
Changed custom expectations to local functions.
Nov 1, 2016
1848de6
Replaced stringToArrayBuffer with utf8Encode.
Jan 1, 2015
59e9634
Fixed visibility assertion.
Nov 1, 2016
0c72a3c
Lint fixes.
Nov 1, 2016
487d149
Added a comment (partly in order to get Travis to re-run).
Nov 2, 2016
fbda8c8
Changes in response to reviews.
Nov 3, 2016
b384663
Rebased and fixed small rebase error.
Nov 3, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Changes in response to reviews.
  • Loading branch information
Terran Lane committed Nov 3, 2016
commit fbda8c8b7da47601df6beb3304e22fe59aaaf2a5
40 changes: 22 additions & 18 deletions extensions/amp-a4a/0.1/test/test-a4a-integration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ function expectRenderedInFriendlyIframe(element, srcdoc) {
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 => {
expect(toTest).to.be.visible;
});
expect(element, 'ad tag').to.be.visible;
expect(child, 'iframe child').to.be.visible;
expect(childBody, 'ad creative content body').to.be.visible;
}

function expectRenderedInXDomainIframe(element, src) {
Expand All @@ -58,9 +58,8 @@ function expectRenderedInXDomainIframe(element, src) {
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 => {
expect(toTest).to.be.visible;
});
expect(element, 'ad tag').to.be.visible;
expect(child, 'iframe child').to.be.visible;
}

describe('integration test: a4a', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Author

Choose a reason for hiding this comment

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

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.

Expand Down Expand Up @@ -148,14 +147,14 @@ describe('integration test: a4a', () => {
});

it('should fall back to 3p when extractCreative returns empty sig', () => {
sandbox.stub(MockA4AImpl.prototype, 'extractCreativeAndSignature')
.onFirstCall().returns({
creative: utf8Encode(validCSSAmp.reserialized),
signature: null,
})
.onSecondCall().throws(new Error(
'Testing extractCreativeAndSignature should not occur error'));
const extractCreativeAndSignatureStub =
sandbox.stub(MockA4AImpl.prototype, 'extractCreativeAndSignature');
extractCreativeAndSignatureStub.onFirstCall().returns({
creative: utf8Encode(validCSSAmp.reserialized),
signature: null,
});
return fixture.addElement(a4aElement).then(unusedElement => {
expect(extractCreativeAndSignatureStub).to.be.calledOnce;
expectRenderedInXDomainIframe(a4aElement, TEST_URL);
});
});
Expand All @@ -178,10 +177,15 @@ describe('integration test: a4a', () => {
expectRenderedInXDomainIframe(a4aElement, TEST_URL);
});
});

// TODO(@ampproject/a4a): Need a test that double-checks that thrown errors
// are propagated out and printed to console and/or sent upstream to error
// logging systems. This is a bit tricky, because it's handled by the AMP
// runtime and can't be done within the context of a
// fixture.addElement().then() or .catch(). This should be integrated into
// all tests, so that we know precisely when errors are being reported and
// to whom.
it('should propagate errors out and report them to upstream error log');
});

// TODO(@ampproject/a4a): Need a test that double-checks that thrown errors
// are propagated out and printed to console and/or sent upstream to error
// logging systems. This is a bit tricky, because it's handled by the AMP
// runtime and can't be done within the context of a
// fixture.addElement().then() or .catch().