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
Changed custom expectations to local functions.
  • Loading branch information
Terran Lane committed Nov 3, 2016
commit 4fd797cf64972aee79117aa415780083afece9f8
85 changes: 23 additions & 62 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 @@ -34,68 +34,29 @@ import {
} from '../../../../src/custom-element';
import * as sinon from 'sinon';

chai.Assertion.addMethod('renderedInFriendlyIframe', function(srcdoc) {
const obj = this._obj;
this.assert(obj,
'amp ad element should be #{exp}, got #{act}', // if true message
'amp ad element should not be #{exp}, got #{act}', // if negated message
'truthy', // expected
obj); // actual
const elementName = obj.tagName.toLowerCase();
const child = obj.querySelector('iframe[srcdoc]');
this.assert(child,
`child of amp ad element ${elementName} should be #{exp}, got #{act}`,
`child of amp ad element ${elementName} should not be #{exp}, got #{act}`,
'truthy',
child);
this.assert(child.getAttribute('srcdoc').indexOf(srcdoc) >= 0,
`iframe child of amp ad element ${elementName} should contain #{exp}, ` +
'was #{act}',
`iframe child of amp ad element ${elementName} should not contain ` +
'#{exp}, was #{act}',
srcdoc,
child.getAttribute('srcdoc'));
function expectRenderedInFriendlyIframe(element, srcdoc) {
expect(element, 'ad element').to.be.ok;
const child = element.querySelector('iframe[srcdoc]');
expect(child, 'iframe child').to.be.ok;
expect(child.getAttribute('srcdoc')).to.contain.string(srcdoc);
const childBody = child.contentDocument.body;
this.assert(childBody,
`iframe child of amp ad element ${elementName} should have #{exp}`,
`iframe child of amp ad element ${elementName} should have #{exp}, ` +
'got #{act}',
'body tag',
childBody);
[obj, child, childBody].forEach(toTest => {
expect(childBody, 'body of iframe doc').to.be.ok;
[element, child, childBody].forEach(toTest => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

expect(toTest).to.be.visible;
});
});
}

chai.Assertion.addMethod('renderedInXDomainIframe', function(src) {
const obj = this._obj;
this.assert(obj,
'amp ad element should be #{exp}, got #{act}', // if true message
'amp ad element should not be #{exp}, got #{act}', // if negated message
'truthy', // expected
obj); // actual
const elementName = obj.tagName.toLowerCase();
const friendlyChild = obj.querySelector('iframe[srcdoc]');
this.assert(!friendlyChild,
`child of amp ad element ${elementName} should not be cross-domain`,
`child of amp ad element ${elementName} should be cross-domain`);
const child = obj.querySelector('iframe[src]');
this.assert(child,
`child of amp ad element ${elementName} should be #{exp}, got #{act}`,
`child of amp ad element ${elementName} should not be #{exp}, got #{act}`,
'truthy',
child);
this.assert(child.getAttribute('src').indexOf(src) >= 0,
`iframe child of amp ad element ${elementName} src should contain ` +
'#{exp}, was #{act}',
`iframe child of amp ad element ${elementName} src should not contain ` +
'#{exp}, was #{act}',
src,
child.getAttribute('src'));
[obj, child].forEach(toTest => {
function expectRenderedInXDomainIframe(element, src) {
expect(element, 'ad element').to.be.ok;
expect(element.querySelector('iframe[srcdoc]'),
'does not have a friendly iframe child').to.not.be.ok;
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 => {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Author

Choose a reason for hiding this comment

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

Done.

expect(toTest).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.

let sandbox;
Expand Down Expand Up @@ -143,14 +104,14 @@ describe('integration test: a4a', () => {

it('should render a single AMP ad in a friendly iframe', () => {
return fixture.addElement(a4aElement).then(element => {
expect(element).to.be.renderedInFriendlyIframe('Hello, world.');
expectRenderedInFriendlyIframe(a4aElement, 'Hello, world.');
});
});

it('should fall back to 3p when no signature is present', () => {
mockResponse.headers.delete(SIGNATURE_HEADER);
return fixture.addElement(a4aElement).then(element => {
expect(element).to.be.renderedInXDomainIframe(TEST_URL);
expectRenderedInXDomainIframe(a4aElement, TEST_URL);
});
});

Expand All @@ -163,7 +124,7 @@ describe('integration test: a4a', () => {
return fixture.addElement(a4aElement).catch(error => {
expect(error.message).to.contain.string('Testing network error');
expect(error.message).to.contain.string('amp-a4a:');
expect(a4aElement).to.be.renderedInXDomainIframe(TEST_URL);
expectRenderedInXDomainIframe(a4aElement, TEST_URL);
});
});

Expand All @@ -177,7 +138,7 @@ describe('integration test: a4a', () => {
expect(error.message).to.contain.string(
'Testing extractCreativeAndSignature error');
expect(error.message).to.contain.string('amp-a4a:');
expect(a4aElement).to.be.renderedInXDomainIframe(TEST_URL);
expectRenderedInXDomainIframe(a4aElement, TEST_URL);
});
});

Expand All @@ -190,7 +151,7 @@ describe('integration test: a4a', () => {
.onSecondCall().throws(new Error(
'Testing extractCreativeAndSignature should not occur error'));
Copy link
Contributor

Choose a reason for hiding this comment

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

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;

Copy link
Author

Choose a reason for hiding this comment

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

Done.

return fixture.addElement(a4aElement).then(element => {
expect(element).to.be.renderedInXDomainIframe(TEST_URL);
expectRenderedInXDomainIframe(a4aElement, TEST_URL);
});
});

Expand All @@ -209,7 +170,7 @@ describe('integration test: a4a', () => {
return fixture.addElement(a4aElement).catch(error => {
expect(error.message).to.contain.string('Key failed to validate');
expect(error.message).to.contain.string('amp-a4a:');
expect(a4aElement).to.be.renderedInXDomainIframe(TEST_URL);
expectRenderedInXDomainIframe(a4aElement, TEST_URL);
});
});
});
Expand Down