-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Description
Background:
In #7381, @cramforce brought up the issue that several of our tests were silently passing in spite of errors being logged by the AMP runtime.
#14336 added a stub for console.error to test/_init_tests.js so that AMP runtime errors during tests would now be surfaced as mocha test errors. It also skipped about 6% of the existing AMP tests that were erroring out due to this change.
Update: Due to the sheer number of tests with errors, #14549 unskipped all the tests that were skipped in #14336. Now, when a test contains a console.error that's not wrapped in allowConsoleError, a warning will be printed with instructions, and the test will continue to pass.
Update: With #15621, there's a new construct called expectAsyncConsoleError('message') that can be used for async errors.
Action item:
Now, it's time to fix these tests. I'm assigning this issue to all component owners whose tests were skipped by #14336. (GH only allows 10 assignees for issues, but I'm assuming the list here will cover all components.)
Instructions:
- Look at https://github.com/ampproject/amphtml/pull/14336/files and search for
#14336in the test files you own. - Unskip all your skipped tests and run them on Travis, or locally using
gulp test --unitorgulp test --integrationorgulp test --local-changes- Sometimes, tests may pass when run in isolation, but fail (or print a message about
console.error) when run with other tests. This is typically a symptom of the test relying on dirty global state.
- Sometimes, tests may pass when run in isolation, but fail (or print a message about
- When a test fails with an error:
- If the error is genuine, fix the runtime code that generated the error.
- If the error is synchronous, and expected during the test, use an
allowConsoleErrorblock around the test step that generates the error. - If the error is asynchronous, and expected during the test, use
expectAsyncConsoleError(message)at the top of the test
Examples (UPDATED, based on #14432 and #15609):
Fails:
it('test with unexpected console errors', () => {
doAThing(); // No console error
doAnotherThing(); // Contains a console error
doYetAnotherThing(); // Contains a console error
});
Fails:
it('test with some unexpected synchronous console errors', () => {
doAThing(); // No console error
allowConsoleError(() => {
doAnotherThing(); // Contains a synchronous console error
});
doYetAnotherThing(); // Contains a console error
});
Fails:
it('test with some unexpected asynchronous console errors', () => {
expectAsyncConsoleError('Foo');
doAThing(); // No console error
doAnotherThing(); // Contains an asynchronous console error 'Foo'
doYetAnotherThing(); // Contains an asynchronous console error 'Bar'
});
Passes:
it('test with all expected synchronous console errors', () => {
doAThing(); // No console error
allowConsoleError(() => {
doAnotherThing(); // Contains a synchronous console error
doYetAnotherThing(); // Contains a synchronous console error
});
});
Passes:
it('test with all expected asynchronous console errors', () => {
expectAsyncConsoleError('Foo');
expectAsyncConsoleError('Bar');
doAThing(); // No console error
doAnotherThing(); // Contains an asynchronous console error 'Foo'
doYetAnotherThing(); // Contains an asynchronous console error 'Bar'
});
Related to #14360