Amphtml: Fix all tests that were silently passing in spite of AMP runtime errors

[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

[rsimha]

Pinging several folks for maximum visibility. (Sorry about the GitHub notifications this might generate.)

Attention (in no particular order): @jridgewell @alanorozco @dvoytenko @keithwrightbos @aghassemi @cvializ @bradfrizzell @zhouyx @lannka @choumx @charliereams @glevitzky @avimehta @jonkeller @nainar @danielrozenberg @mkhatib @cathyxz @prateekbh @tiendao @newmuis

You were mentioned in this issue because you wrote, edited, or reviewed a test that had to be skipped because it contained console errors from the AMP runtime that are now being surfaced as test failures.

If you see a skipped test with TODO(xxxx, #14336) that's incorrectly assigned to you in this PR, please ping whoever you think ought to fix that test.

[rsimha]

Update: With #14432, there are a few changes to note:

• We now use sinon.mock instead of sinon.stub to keep track of when console.error is called during tests.
• We no longer throw an error during tests when console.error is detected (some tests have legitimate reasons to do so). Instead, we check for console.error in the global afterEach.
• Tests can wrap code that legitimately calls console.error within an allowConsoleError block (see updated examples above).
• For now, tests that call console.error without wrapping the call in allowConsoleError will print a warning immediately after the test (when run with --files) / at the end of all tests (when all tests are run).
• Once all existing instances of console.error are fixed or wrapped in allowConsoleError, we can switch to failing tests with errors.
• Here is an example of error messages you might see during local development

[rsimha]

Update: With #14549, all the tests that were skipped in #14336 are now re-enabled. When a test contains a console.error that's not wrapped in allowConsoleError, a warning will be printed with instructions. Tests will continue to pass for now.

Once all errors are cleaned up, we will go back to failing tests with unexpected errors.

[choumx]

Should this be closed now that the TODOs are removed and console errors fail locally?

[rsimha]

@choumx We still have about 700 unfixed / uncaught errors when all unit tests are run: https://travis-ci.org/ampproject/amphtml/jobs/383335338#L2306. This is why we still warn on Travis.

I was going to keep this issue open until all those are fixed, and we move from warning to failing tests on Travis. WDYT?

[rsimha]

With #15621, there's a new construct called expectAsyncConsoleError('message') that can be used for async errors.

[lannka]

I found rethrowAsync is a major culprit of this issue. And none of the existing solution worked for that, since it does not only print console errors but also throw an exception in the next test.

The current console error stub somehow completely shielded the exception from rethrowAsync and silently skipped the remaining tests. (I tried that removing the console error stub will surface the exception message to test log).

One solution is to stub rethrowAsync in test.

[rsimha]

@lannka Nice find! Would it make sense to throw the error synchronously during tests? That way, we won't lose the error information, and we will have a complete call stack in the test logs.

[lannka]

@rsimha agreed. The only concern is that then there is no way to verify a fallback logic of such an error.

if (bad) {
  asyncThrow();
  return 'fallback';
}
return 'good';

We will not be able to have a test to verify "fallback".

[rsimha]

We no longer rethrow (synchronously or asynchronously) when an uncaught console error is detected in a test.