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

Jump to bottom

Make bind integration tests less flaky (the remix) #9788

Merged
merged 3 commits into from Jun 8, 2017
Merged

Make bind integration tests less flaky (the remix) #9788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0ef9239
wait for form service init in integration test
Jun 7, 2017
a4ce003
wait for element layouts for each fixture
Jun 7, 2017
de838a6
fix format errors in bind-list.html, remove unnecessary ampdoc refs
Jun 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions extensions/amp-form/0.1/amp-form.js
View file
Open in desktop
Expand Up @@ -894,6 +894,16 @@ export class AmpFormService {
/** @const @private {!Promise} */
this.whenInitialized_ = this.installStyles_(ampdoc)
.then(() => this.installHandlers_(ampdoc));

// Dispatch a test-only event for integration tests.
if (getMode().test) {
this.whenInitialized_.then(() => {
const win = ampdoc.win;
const event = createCustomEvent(
win, 'amp:form-service:initialize', null, {bubbles: true});
win.dispatchEvent(event);
});
}
}

/**
Expand Down
5 changes: 0 additions & 5 deletions test/fixtures/bind-brightcove.html
View file
Open in desktop
Expand Up @@ -9,11 +9,6 @@
<script async src="/dist/amp.js"></script>
<script async custom-element="amp-bind" src="/dist/v0/amp-bind-0.1.max.js"></script>
<script async custom-element="amp-brightcove" src="/dist/v0/amp-brightcove-0.1.max.js"></script>
<style amp-custom>
amp-brightcove {
margin-top: 600px; /* iframes must not be in initial viewport as a rule */
}
</style>
</head>
<body>
<!-- amp-brightcove -->
Expand Down
5 changes: 2 additions & 3 deletions test/fixtures/bind-carousel.html
View file
Open in desktop
Expand Up @@ -16,9 +16,8 @@
<p id="slideNumber" [text]="selectedSlide">0</p>
<amp-carousel width="300" height="100" type="slides" id="carousel"
on="slideChange:AMP.setState({selectedSlide: event.index})" [slide]="selectedSlide">
<amp-img src="http://www.google.com/image1" height="200" width="200"></amp-img>
<amp-img src="http://www.google.com/image1" height="200" width="200"></amp-img>
<amp-img src="http://www.google.com/image1" height="200" width="200"></amp-img>
<amp-img src="http://www.foo.com/image" height="200" width="200"></amp-img>
<amp-img src="http://www.foo.com/image" height="200" width="200"></amp-img>
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious: Why the intentionally invalid images?

Copy link
Author

Choose a reason for hiding this comment

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

Real, locally served images would avoid polluting the error log but it doesn't affect test correctness either way. Would be a good clean-up task or GFI.

</amp-carousel>
</body>
</html>
7 changes: 1 addition & 6 deletions test/fixtures/bind-iframe.html
View file
Open in desktop
Expand Up @@ -9,18 +9,13 @@
<script async src="/dist/amp.js"></script>
<script async custom-element="amp-bind" src="/dist/v0/amp-bind-0.1.max.js"></script>
<script async custom-element="amp-iframe" src="/dist/v0/amp-iframe-0.1.max.js"></script>
<style amp-custom>
amp-iframe {
margin-top: 600px; /* iframes must not be in initial viewport as a rule */
}
</style>
</head>

<body>
<!-- amp-iframe -->
<button id="iframeButton" on="tap:AMP.setState({iframe: 'https://giphy.com/embed/DKG1OhBUmxL4Q'})">Change amp-iframe</button>
<amp-iframe width=100 height=100 id="ampIframe" sandbox="allow-scripts allow-same-origin allow-popups"
src="https://player.vimeo.com/video/140261016" [src]="iframe">
<amp-img layout="fill" src="https://foo.com/foo.png" placeholder></amp-img>
</amp-iframe>
</body>
</html>
6 changes: 3 additions & 3 deletions test/fixtures/bind-list.html
View file
Open in desktop
Expand Up @@ -13,9 +13,9 @@

<body>
<!-- amp-list -->
<button id='listSrcButton' on="tap:AMP.setState({listSrc: 'https://www.google.com/bound.json'})">Change amp-list</button>
<button id='httpListSrcButton' on="tap:AMP.setState({listSrc: 'http://www.google.com/justhttp.json'})">Change amp-list</button>
<amp-list id='list' src="https://www.google.com/unbound.json" [src]=listSrc>
<button id="listSrcButton" on="tap:AMP.setState({listSrc: 'https://www.google.com/bound.json'})">Change amp-list</button>
<button id="httpListSrcButton" on="tap:AMP.setState({listSrc: 'http://www.google.com/justhttp.json'})">Change amp-list</button>
<amp-list id="list" width=100 height=100 src="https://www.google.com/unbound.json" [src]=listSrc>
</amp-list>
</body>
</html>
52 changes: 26 additions & 26 deletions test/integration/test-amp-bind.js
View file
Open in desktop
Expand Up @@ -15,13 +15,11 @@
*/

import {createFixtureIframe} from '../../testing/iframe';
import {batchedXhrFor, bindForDoc} from '../../src/services';
import {ampdocServiceFor} from '../../src/ampdoc';
import {batchedXhrFor} from '../../src/services';
import * as sinon from 'sinon';

describe.configure().retryOnSaucelabs().run('amp-bind', function() {
let fixture;
let ampdoc;
let sandbox;
let numSetStates;
let numTemplated;
Expand All @@ -40,30 +38,30 @@ describe.configure().retryOnSaucelabs().run('amp-bind', function() {

/**
* @param {string} fixtureLocation
* @param {number=} opt_numberOfAmpElements
* @return {!Promise}
*/
function setupWithFixture(fixtureLocation) {
function setupWithFixture(fixtureLocation, opt_numberOfAmpElements) {
return createFixtureIframe(fixtureLocation).then(f => {
fixture = f;
return fixture.awaitEvent('amp:bind:initialize', 1);
}).then(() => {
const ampdocService = ampdocServiceFor(fixture.win);
ampdoc = ampdocService.getAmpDoc(fixture.doc);
// Most fixtures have a single AMP element that will be laid out.
const loadStartsToExpect =
(opt_numberOfAmpElements === undefined) ? 1 : opt_numberOfAmpElements;
return Promise.all([
fixture.awaitEvent('amp:bind:initialize', 1),
fixture.awaitEvent('amp:load:start', loadStartsToExpect),
]);
});
}

/** @return {!Promise} */
function waitForBindApplication() {
// Bind should be available, but need to wait for actions to resolve
// service promise for bind and call setState.
return bindForDoc(ampdoc).then(unusedBind =>
fixture.awaitEvent('amp:bind:setState', ++numSetStates));
return fixture.awaitEvent('amp:bind:setState', ++numSetStates);
}

/** @return {!Promise} */
function waitForTemplateRescan() {
return bindForDoc(ampdoc).then(unusedBind =>
fixture.awaitEvent('amp:bind:rescan-template', ++numTemplated));
return fixture.awaitEvent('amp:bind:rescan-template', ++numTemplated);
}

describe('with [text] and [class]', () => {
Expand Down Expand Up @@ -92,10 +90,14 @@ describe.configure().retryOnSaucelabs().run('amp-bind', function() {
});
});

// TODO(choumx, #9759): Remove Chrome-only condition.
// TODO(choumx, #9759): Seems like old browsers give up when hitting expected
// user errors due to illegal bindings in the form's template.
describe.configure().ifChrome().run('with <amp-form>', () => {
beforeEach(() => {
return setupWithFixture('test/fixtures/bind-form.html');
// <form> is not an AMP element.
return setupWithFixture('test/fixtures/bind-form.html', 0)
// Wait for AmpFormService to register <form> elements.
.then(() => fixture.awaitEvent('amp:form-service:initialize', 1));
});

it('should NOT allow invalid bindings or values', () => {
Expand Down Expand Up @@ -191,10 +193,11 @@ describe.configure().retryOnSaucelabs().run('amp-bind', function() {
});
});

// TODO(choumx): Flaky on Edge for some reason.
describe.configure().skipEdge().run('with <amp-carousel>', () => {
// TODO(choumx): Flaky on Edge/Firefox for some reason.
describe.configure().ifChrome().run('with <amp-carousel>', () => {
beforeEach(() => {
return setupWithFixture('test/fixtures/bind-carousel.html');
// One <amp-carousel> plus two <amp-img> elements.
return setupWithFixture('test/fixtures/bind-carousel.html', 3);
});

it('should update on carousel slide changes', () => {
Expand Down Expand Up @@ -353,7 +356,8 @@ describe.configure().retryOnSaucelabs().run('amp-bind', function() {

describe('with <amp-selector>', () => {
beforeEach(() => {
return setupWithFixture('test/fixtures/bind-selector.html');
// One <amp-selector> and three <amp-img> elements.
return setupWithFixture('test/fixtures/bind-selector.html', 4);
});

it('should update when selection changes', () => {
Expand Down Expand Up @@ -489,8 +493,6 @@ describe.configure().retryOnSaucelabs().run('amp-bind', function() {
it('should support binding to data-account', () => {
const button = fixture.doc.getElementById('brightcoveButton');
const bc = fixture.doc.getElementById('brightcove');
// Force layout in case element is not in viewport.
bc.implementation_.layoutCallback();
const iframe = bc.querySelector('iframe');
expect(iframe.src).to.not.contain('bound');
button.click();
Expand All @@ -502,16 +504,14 @@ describe.configure().retryOnSaucelabs().run('amp-bind', function() {

describe('with <amp-iframe>', () => {
beforeEach(() => {
return setupWithFixture('test/fixtures/bind-iframe.html');
// <amp-iframe> and its placeholder <amp-img>.
return setupWithFixture('test/fixtures/bind-iframe.html', 2);
});

it('should support binding to src', () => {
const button = fixture.doc.getElementById('iframeButton');
const ampIframe = fixture.doc.getElementById('ampIframe');
// Force layout in case element is not in viewport.
ampIframe.implementation_.layoutCallback();
const iframe = ampIframe.querySelector('iframe');

const newSrc = 'https://giphy.com/embed/DKG1OhBUmxL4Q';
expect(ampIframe.getAttribute('src')).to.not.contain(newSrc);
expect(iframe.src).to.not.contain(newSrc);
Expand Down
1 change: 1 addition & 0 deletions testing/iframe.js
View file
Open in desktop
Expand Up @@ -69,6 +69,7 @@ export function createFixtureIframe(fixture, initialIframeHeight, opt_beforeLoad
'amp:bind:setState': 0,
'amp:bind:rescan-template': 0,
'amp:error': 0,
'amp:form-service:initialize': 0,
'amp:load:start': 0,
'amp:stubbed': 0,
};
Expand Down