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

🏗🐛 Prevent most async throwing of errors during tests #16916

Merged
merged 3 commits into from
Jul 25, 2018
Merged

🏗🐛 Prevent most async throwing of errors during tests #16916

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
f17cf10
Make error throwing synchronous during tests
rsimha Jul 19, 2018
0eb1f0d
Fix / skip a few tests
rsimha Jul 20, 2018
e3e4b37
Address feedback
rsimha Jul 24, 2018
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
4 changes: 3 additions & 1 deletion .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@
"global": false,
"describes": true,
"allowConsoleError": false,
"expectAsyncConsoleError": false
"expectAsyncConsoleError": false,
"restoreAsyncErrorThrows": false,
"stubAsyncErrorThrows": false
},
"settings": {
"jsdoc": {
Expand Down
6 changes: 2 additions & 4 deletions extensions/amp-analytics/0.1/test/test-amp-analytics.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,12 +486,10 @@ describes.realWin('amp-analytics', {
});

it('should tolerate invalid triggers', function() {
const clock = sandbox.useFakeTimers();
const analytics = getAnalyticsTag();
// An incomplete click request.
analytics.addTriggerNoInline_({'on': 'click'});
allowConsoleError(() => { expect(() => {
clock.tick(1);
// An incomplete click request.
analytics.addTriggerNoInline_({'on': 'click'});
}).to.throw(/Failed to process trigger/); });
});

Expand Down
6 changes: 4 additions & 2 deletions extensions/amp-app-banner/0.1/test/test-amp-app-banner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,8 @@ describes.realWin('amp-app-banner', {

it('should throw if open button is missing', testButtonMissing);

it('should remove banner if already dismissed',
// TODO(#16916): Make this test work with synchronous throws.
it.skip('should remove banner if already dismissed',
testRemoveBannerIfDismissed);

it('should remove banner if meta is not provided', () => {
Expand Down Expand Up @@ -249,7 +250,8 @@ describes.realWin('amp-app-banner', {

it('should throw if open button is missing', testButtonMissing);

it('should remove banner if already dismissed',
// TODO(#16916): Make this test work with synchronous throws.
it.skip('should remove banner if already dismissed',
testRemoveBannerIfDismissed);

it('should remove banner if manifest is not provided', () => {
Expand Down
3 changes: 2 additions & 1 deletion extensions/amp-sticky-ad/1.0/test/test-amp-sticky-ad.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ describes.realWin('amp-sticky-ad 1.0 version', {
() => addToFixedLayerPromise);
});

it('should listen to scroll event', function * () {
// TODO(#16916): Make this test work with synchronous throws.
it.skip('should listen to scroll event', function * () {
const spy = sandbox.spy(impl, 'removeOnScrollListener_');
expect(impl.scrollUnlisten_).to.be.null;
yield macroTask();
Expand Down
4 changes: 2 additions & 2 deletions src/log.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,9 +507,9 @@ export function duplicateErrorIfNecessary(error) {
/**
* @param {...*} var_args
* @return {!Error}
* @private
* @visibleForTesting
*/
function createErrorVargs(var_args) {
export function createErrorVargs(var_args) {
let error = null;
let message = '';
for (let i = 0; i < arguments.length; i++) {
Expand Down
29 changes: 26 additions & 3 deletions test/_init_tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import '../src/polyfills';
import 'babel-polyfill';
import * as describes from '../testing/describes';
import * as log from '../src/log';
import {Services} from '../src/services';
import {activateChunkingForTesting} from '../src/chunk';
import {
Expand Down Expand Up @@ -46,6 +47,7 @@ let consoleErrorStub;
let consoleInfoLogWarnSandbox;
let testName;
let expectedAsyncErrors;
let rethrowAsyncSandbox;

// Used to clean up global state between tests.
let initialGlobalState;
Expand Down Expand Up @@ -293,7 +295,6 @@ function warnForConsoleError() {
expectedAsyncErrors = [];
consoleErrorSandbox = sinon.sandbox.create();
const originalConsoleError = console/*OK*/.error;
setReportError(() => {});
consoleErrorSandbox.stub(console, 'error').callsFake((...messages) => {
const message = messages.join(' ');

Expand Down Expand Up @@ -369,7 +370,6 @@ function dontWarnForConsoleError() {
consoleErrorSandbox = sinon.sandbox.create();
consoleErrorStub =
consoleErrorSandbox.stub(console, 'error').callsFake(() => {});
setReportError(reportError);
}

// Used to restore error level logging after each test.
Expand Down Expand Up @@ -403,6 +403,28 @@ function restoreConsoleInfoLogWarn() {
}
}

// Used to precent asynchronous throwing of errors during each test.
function preventAsyncErrorThrows() {
this.stubAsyncErrorThrows = function() {
if (rethrowAsyncSandbox) {
rethrowAsyncSandbox.restore();
}
rethrowAsyncSandbox = sinon.sandbox.create();
rethrowAsyncSandbox.stub(log, 'rethrowAsync').callsFake((...args) => {
const error = log.createErrorVargs.apply(null, args);
self.reportError(error);
throw error;
});
};
this.restoreAsyncErrorThrows = function() {
if (rethrowAsyncSandbox) {
rethrowAsyncSandbox.restore();
}
};
setReportError(reportError);
stubAsyncErrorThrows();
}

beforeEach(function() {
this.timeout(BEFORE_AFTER_TIMEOUT);
beforeTest();
Expand All @@ -411,6 +433,7 @@ beforeEach(function() {
if (!verboseLogging) {
stubConsoleInfoLogWarn();
}
preventAsyncErrorThrows();
warnForConsoleError();
initialGlobalState = Object.keys(global);
initialWindowState = Object.keys(window);
Expand Down Expand Up @@ -438,6 +461,7 @@ afterEach(function() {
const windowState = Object.keys(window);
restoreConsoleError();
restoreConsoleInfoLogWarn();
restoreAsyncErrorThrows();
this.timeout(BEFORE_AFTER_TIMEOUT);
const cleanupTagNames = ['link', 'meta'];
if (!Services.platformFor(window).isSafari()) {
Expand Down Expand Up @@ -495,7 +519,6 @@ afterEach(function() {
resetAccumulatedErrorMessagesForTesting();
resetExperimentTogglesForTesting(window);
resetEvtListenerOptsSupportForTesting();
setReportError(reportError);
});

chai.Assertion.addMethod('attribute', function(attr) {
Expand Down
14 changes: 5 additions & 9 deletions test/functional/test-3p.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,23 +138,19 @@ describe('3p', () => {
location: true,
mode: true,
}, /* mandatory */[], /* optional */[]);
clock.tick(1);

validateData({
width: '',
foo: true,
bar: true,
}, /* mandatory */[], ['foo', 'bar']);
clock.tick(1);

validateData({
type: 'TEST',
foo: true,
'not-whitelisted': true,
}, [], ['foo']);

allowConsoleError(() => { expect(() => {
clock.tick(1);
validateData({
type: 'TEST',
foo: true,
'not-whitelisted': true,
}, [], ['foo']);
}).to.throw(/Unknown attribute for TEST: not-whitelisted./); });
});

Expand Down
6 changes: 0 additions & 6 deletions test/functional/test-cid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
* limitations under the License.
*/

import * as log from '../../src/log';
import * as lolex from 'lolex';
import * as sinon from 'sinon';
import * as url from '../../src/url';
Expand Down Expand Up @@ -789,7 +788,6 @@ describes.realWin('cid', {amp: true}, env => {
sandbox.stub(url, 'isProxyOrigin').returns(true);
let scopedCid = undefined;
let resolved = false;
const rethrowAsyncStub = sandbox.stub(log, 'rethrowAsync');
cid.get({scope: 'foo'}, hasConsent)
.then(result => {
scopedCid = result;
Expand All @@ -799,12 +797,8 @@ describes.realWin('cid', {amp: true}, env => {
clock.tick(9999);
yield macroTask();
expect(resolved).to.be.false;
clock.tick(1);
yield macroTask();
expect(resolved).to.be.true;
expect(scopedCid).to.be.undefined;
yield macroTask();
expect(rethrowAsyncStub).to.be.calledOnce;
});

describe('pub origin, CID API opt in', () => {
Expand Down
12 changes: 8 additions & 4 deletions test/functional/test-extensions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,8 @@ describes.sandboxed('Extensions', {}, () => {
expect(holder.docFactories[0]).to.equal(factory);
});

it('should install all doc factories to shadow doc', () => {
// TODO(#16916): Make this test work with synchronous throws.
it.skip('should install all doc factories to shadow doc', () => {
sandbox.stub(Services.ampdocServiceFor(win), 'isSingleDoc').callsFake(
() => false);
const factory1 = sandbox.spy();
Expand Down Expand Up @@ -495,7 +496,8 @@ describes.sandboxed('Extensions', {}, () => {
expect(getServiceForDoc(ampdoc, 'service2').a).to.equal(2);
});

it('should install all services to doc', () => {
// TODO(#16916): Make this test work with synchronous throws.
it.skip('should install all services to doc', () => {
sandbox.stub(Services.ampdocServiceFor(win), 'isSingleDoc').callsFake(
() => false);
const factory1 = sandbox.spy();
Expand Down Expand Up @@ -815,7 +817,8 @@ describes.sandboxed('Extensions', {}, () => {
});
});

it('should survive factory failures', () => {
// TODO(#16916): Make this test work with synchronous throws.
it.skip('should survive factory failures', () => {
const factory1Spy = sandbox.spy();
const factory2Spy = sandbox.spy();
const factory3Spy = sandbox.spy();
Expand Down Expand Up @@ -1001,7 +1004,8 @@ describes.sandboxed('Extensions', {}, () => {
});
});

it('should call pre-install callback before other installs', () => {
// TODO(#16916): Make this test work with synchronous throws.
it.skip('should call pre-install callback before other installs', () => {
let preinstallCount = 0;
const extHolder = extensions.getExtensionHolder_('amp-test');
extHolder.scriptPresent = true;
Expand Down
3 changes: 2 additions & 1 deletion test/functional/test-friendly-iframe-embed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,7 +682,8 @@ describe('friendly-iframe-embed', () => {
});
});

it('should stop polling when loading failed', () => {
// TODO(#16916): Make this test work with synchronous throws.
it.skip('should stop polling when loading failed', () => {
iframe.contentWindow = contentWindow;
const embedPromise = installFriendlyIframeEmbed(iframe, container, {
url: 'https://acme.org/url1',
Expand Down
5 changes: 5 additions & 0 deletions test/functional/test-log.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,11 @@ describe('Logging', () => {

beforeEach(() => {
clock = sandbox.useFakeTimers();
restoreAsyncErrorThrows();
});

afterEach(() => {
stubAsyncErrorThrows();
});

it('should rethrow error with single message', () => {
Expand Down
26 changes: 18 additions & 8 deletions test/functional/test-url-replacements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,14 +546,16 @@ describes.sandboxed('UrlReplacements', {}, () => {
});
});

it('should replace VARIANT', () => {
// TODO(#16916): Make this test work with synchronous throws.
it.skip('should replace VARIANT', () => {
return expect(expandUrlAsync(
'?x1=VARIANT(x1)&x2=VARIANT(x2)&x3=VARIANT(x3)',
/*opt_bindings*/undefined, {withVariant: true}))
.to.eventually.equal('?x1=v1&x2=none&x3=');
});

it('should replace VARIANT with empty string if ' +
// TODO(#16916): Make this test work with synchronous throws.
it.skip('should replace VARIANT with empty string if ' +
'amp-experiment is not configured ', () => {
return expect(expandUrlAsync(
'?x1=VARIANT(x1)&x2=VARIANT(x2)&x3=VARIANT(x3)'))
Expand All @@ -565,7 +567,8 @@ describes.sandboxed('UrlReplacements', {}, () => {
{withVariant: true})).to.eventually.equal('?x1.v1!x2.none');
});

it('should replace VARIANTS with empty string if ' +
// TODO(#16916): Make this test work with synchronous throws.
it.skip('should replace VARIANTS with empty string if ' +
'amp-experiment is not configured ', () => {
return expect(expandUrlAsync('?VARIANTS')).to.eventually.equal('?');
});
Expand All @@ -579,7 +582,8 @@ describes.sandboxed('UrlReplacements', {}, () => {
.to.eventually.equal('?in=12345&out=54321');
});

it('should replace SHARE_TRACKING_INCOMING and SHARE_TRACKING_OUTGOING' +
// TODO(#16916): Make this test work with synchronous throws.
it.skip('should replace SHARE_TRACKING_INCOMING and SHARE_TRACKING_OUTGOING' +
' with empty string if amp-share-tracking is not configured', () => {
return expect(
expandUrlAsync(
Expand All @@ -594,7 +598,8 @@ describes.sandboxed('UrlReplacements', {}, () => {
.to.eventually.equal('?index=546&id=id-123');
});

it('should replace STORY_PAGE_INDEX and STORY_PAGE_ID' +
// TODO(#16916): Make this test work with synchronous throws.
it.skip('should replace STORY_PAGE_INDEX and STORY_PAGE_ID' +
' with empty string if amp-story is not configured', () => {
return expect(
expandUrlAsync('?index=STORY_PAGE_INDEX&id=STORY_PAGE_ID'))
Expand Down Expand Up @@ -1012,7 +1017,9 @@ describes.sandboxed('UrlReplacements', {}, () => {
.to.eventually.equal('?a=b&b=b');
});

it('should report errors & replace them with empty string (sync)', () => {
// TODO(#16916): Make this test work with synchronous throws.
it.skip('should report errors & replace them with empty ' +
'string (sync)', () => {
const clock = sandbox.useFakeTimers();
const replacements = Services.urlReplacementsForDoc(window.document);
replacements.getVariableSource().set('ONE', () => {
Expand All @@ -1026,7 +1033,9 @@ describes.sandboxed('UrlReplacements', {}, () => {
return p;
});

it('should report errors & replace them with empty string (promise)', () => {
// TODO(#16916): Make this test work with synchronous throws.
it.skip('should report errors & replace them with empty ' +
'string (promise)', () => {
const clock = sandbox.useFakeTimers();
const replacements = Services.urlReplacementsForDoc(window.document);
replacements.getVariableSource().set('ONE', () => {
Expand Down Expand Up @@ -1268,7 +1277,8 @@ describes.sandboxed('UrlReplacements', {}, () => {
});
});

it('should collect unwhitelisted vars', () => {
// TODO(#16916): Make this test work with synchronous throws.
it.skip('should collect unwhitelisted vars', () => {
const win = getFakeWindow();
const element = document.createElement('amp-foo');
element.setAttribute('src', '?SOURCE_HOST&QUERY_PARAM(p1)&COUNTER');
Expand Down
3 changes: 2 additions & 1 deletion test/functional/test-xhr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,8 @@ describe.configure().skipSafari().run('XHR', function() {
});
});

it('should do simple JSON fetch', () => {
// TODO(#16916): Make this test work with synchronous throws.
it.skip('should do simple JSON fetch', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I have no explanation why but after this, it silently fails rest of the tests in the file.

What should have been an execution of 100 tests altogether, are now just 9 tests but passing

results via locally running gulp test --files=test/functional/test-xhr.js --watch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you hit the debug button in the karma window, do you see any errors when the tests are re run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prateekbh I was able to repro your problem. One of your tests is throwing an error in beforeEach(). Mocha considers this a catastrophic error and bails. Let me know if you need help reproing it.

Copy link
Member

@prateekbh prateekbh Jul 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

well i can use some help on how to find whats the issue, I am creating a PR which just touched xhr-impl.js and hence I reproduced this error.
Not really sure about the setup and test cases here in functional

sandbox.stub(user(), 'assert');
return xhr.fetchJson('http://localhost:31862/get?k=v1')
.then(res => res.json())
Expand Down