Skip to content

Fix all tests that were silently passing in spite of AMP runtime errors #14406

@rsimha

Description

@rsimha

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:

  1. Look at https://github.com/ampproject/amphtml/pull/14336/files and search for #14336 in the test files you own.
  2. Unskip all your skipped tests and run them on Travis, or locally using gulp test --unit or gulp test --integration or gulp 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.
  3. 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 allowConsoleError block 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

Metadata

Metadata

Type

No type

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions