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

Deflake amp-form tests #9384

Merged
merged 2 commits into from May 17, 2017
Merged

Deflake amp-form tests #9384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba3a72e
Deflake amp-form tests
May 16, 2017
bf822c7
Use assert.fail instead of expect(false).to.be.true
May 17, 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
21 changes: 9 additions & 12 deletions extensions/amp-form/0.1/amp-form.js
View file
Open in desktop
Expand Up @@ -29,7 +29,7 @@ import {
isProxyOrigin,
parseUrl,
} from '../../../src/url';
import {dev, user, rethrowAsync} from '../../../src/log';
import {dev, user} from '../../../src/log';
import {getMode} from '../../../src/mode';
import {xhrFor} from '../../../src/services';
import {toArray} from '../../../src/types';
Expand Down Expand Up @@ -475,27 +475,24 @@ export class AmpForm {
this.renderTemplate_(json || {});
this.maybeHandleRedirect_(response);
}, error => {
const message = 'Failed to parse response JSON:';
user().error(TAG, message, error);
rethrowAsync(message, error);
user().error(TAG, `Failed to parse response JSON: ${error}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

  • @jridgewell
    Carlos, Justin:
    do we know why did we had rethrowAsync here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My theory is it was originally there to throw the error outside of the Promise chain so it would show up in the console. Since user().error performs that function too, it was equivalent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope.

});
}

/**
* Transition the form the the submit error state.
* @param {../../../src/service/xhr-impl.FetchError} error
* @param {../../../src/service/xhr-impl.FetchError} errorResponse
* @private
*/
handleXhrSubmitFailure_(error) {
handleXhrSubmitFailure_(errorResponse) {
const error = (errorResponse && errorResponse.error) || errorResponse;
this.triggerAction_(
/* success */ false, error ? error.responseJson : null);
/* success */ false, errorResponse ? errorResponse.responseJson : null);
this.analyticsEvent_('amp-form-submit-error');
this.setState_(FormState_.SUBMIT_ERROR);
this.renderTemplate_(error.responseJson || {});
this.maybeHandleRedirect_(error.response);
const message = 'Form submission failed:';
user().error(TAG, message, error);
rethrowAsync(message, error);
this.renderTemplate_(errorResponse.responseJson || {});
this.maybeHandleRedirect_(errorResponse.response);
user().error(TAG, `Form submission failed: ${error}`);
}

/** @private */
Expand Down
49 changes: 29 additions & 20 deletions extensions/amp-form/0.1/test/test-amp-form.js
View file
Open in desktop
Expand Up @@ -40,6 +40,7 @@ import {
import {FetchError} from '../../../../src/service/xhr-impl';
import '../../../amp-selector/0.1/amp-selector';
import {toggleExperiment} from '../../../../src/experiments';
import {user} from '../../../../src/log';

describes.repeated('', {
'single ampdoc': {ampdoc: 'single'},
Expand Down Expand Up @@ -758,7 +759,8 @@ describes.repeated('', {
expect(form.className).to.not.contain('amp-form-submit-error');
expect(form.className).to.not.contain('amp-form-submit-success');
fetchResolver({json: () => Promise.resolve()});
return timer.promise(5).then(() => {

return ampForm.xhrSubmitPromiseForTesting().then(() => {
expect(ampForm.actions_.trigger).to.be.called;
expect(ampForm.actions_.trigger).to.be.calledWith(form, 'submit');
expect(ampForm.state_).to.equal('submit-success');
Expand All @@ -771,6 +773,8 @@ describes.repeated('', {
/** CustomEvent */ sinon.match.has('detail'));
expect(ampForm.analyticsEvent_).to.be.calledWith(
'amp-form-submit-success');
}, () => {
assert.fail('Submit should have succeeded.');
});
});
});
Expand Down Expand Up @@ -804,7 +808,10 @@ describes.repeated('', {
expect(form.className).to.not.contain('amp-form-submit-error');
expect(form.className).to.not.contain('amp-form-submit-success');
fetchRejecter();
return timer.promise(5).then(() => {

return ampForm.xhrSubmitPromiseForTesting().then(() => {
assert.fail('Submit should have failed.');
}, () => {
expect(button1.hasAttribute('disabled')).to.be.false;
expect(button2.hasAttribute('disabled')).to.be.false;
expect(ampForm.state_).to.equal('submit-error');
Expand Down Expand Up @@ -1505,7 +1512,8 @@ describes.repeated('', {
sandbox.stub(ampForm.xhr_, 'fetch').returns(fetchResolvePromise);
redirectToValue = 'https://google.com/';
ampForm.handleSubmitAction_();
return timer.promise(10).then(() => {

return ampForm.xhrSubmitPromiseForTesting().then(() => {
expect(env.win.top.location.href).to.be.equal(redirectToValue);
});
});
Expand All @@ -1514,7 +1522,10 @@ describes.repeated('', {
sandbox.stub(ampForm.xhr_, 'fetch').returns(fetchResolvePromise);
redirectToValue = 'http://google.com/';
ampForm.handleSubmitAction_();
return timer.promise(10).then(() => {

return ampForm.xhrSubmitPromiseForTesting().then(() => {
assert.fail('Submit should have failed.');
}, () => {
expect(env.win.top.location.href).to.be.equal(
'https://example-top.com/');
});
Expand All @@ -1524,7 +1535,10 @@ describes.repeated('', {
sandbox.stub(ampForm.xhr_, 'fetch').returns(fetchResolvePromise);
redirectToValue = '/hello';
ampForm.handleSubmitAction_();
return timer.promise(10).then(() => {

return ampForm.xhrSubmitPromiseForTesting().then(() => {
assert.fail('Submit should have failed.');
}, () => {
expect(env.win.top.location.href).to.be.equal(
'https://example-top.com/');
});
Expand All @@ -1535,7 +1549,10 @@ describes.repeated('', {
sandbox.stub(ampForm.xhr_, 'fetch').returns(fetchResolvePromise);
redirectToValue = 'http://google.com/';
ampForm.handleSubmitAction_();
return timer.promise(10).then(() => {

return ampForm.xhrSubmitPromiseForTesting().then(() => {
assert.fail('Submit should have failed.');
}, () => {
expect(env.win.top.location.href).to.be.equal(
'https://example-top.com/');
});
Expand All @@ -1544,21 +1561,13 @@ describes.repeated('', {
it('should redirect on error and header is set', () => {
sandbox.stub(ampForm.xhr_, 'fetch').returns(fetchRejectPromise);
redirectToValue = 'https://example2.com/hello';
const errors = [];
const realSetTimeout = window.setTimeout;
sandbox.stub(window, 'setTimeout', (callback, delay) => {
realSetTimeout(() => {
try {
callback();
} catch (e) {
errors.push(e);
}
}, delay);
});
const logSpy = sandbox.spy(user(), 'error');
ampForm.handleSubmitAction_();
return timer.promise(10).then(() => {
expect(errors.length).to.equal(1);
expect(errors[0]).to.match(/Form submission failed/);

return ampForm.xhrSubmitPromiseForTesting().then(() => {
expect(logSpy).to.be.calledOnce;
const error = logSpy.getCall(0).args[1];
expect(error).to.match(/Form submission failed/);
expect(env.win.top.location.href).to.be.equal(redirectToValue);
});
});
Expand Down