🏗🐛 Print uncaught errors seen during tests (instead of rethrowing them)

[rsimha]

We've fought a long battle against uncaught errors in tests (starting with #7381). The solution put in place a while ago (#14336) involved allowing tests to declare errors that are expected during execution, and surfacing ones that weren't declared. Until now, the way we did so was to rethrow the errors (synchronously) so that the test would fail.

However, due to the large number of asynchronous code paths in the AMP runtime, It's not guaranteed that uncaught errors are thrown during the test body. Sometimes, they appear in between tests, which works badly with mocha (#15658), and causes subsequent tests to be silently skipped. While this doesn't happen during full Travis runs, it does happen during local development, and has proven to be very annoying. For example, see #16916 (review) and #17209 (review).

Given this context, this PR does the following:

1. Removes all infrastructure code paths that cause tests to throw when uncaught errors are detected.
2. Instead of throwing, we print the same error message as before, but pass the test.
   • During local development, error messages are printed inline, so developers can address them even though the tests that generate them will now pass
   • During Travis runs, error messages are collected and printed at the end of the test run (in a collapsed section of the logs)

In effect, this PR is backing off from the original policy laid out by @cramforce in #7381 (comment) (to fail tests that generate uncaught errors) due to the high cost of dealing with the resulting failures. However, the mechanism to detect these errors is still in place, and the hope is that printing them out during local development provides sufficient incentive to fix them.

Here's an example of a test that generates an unexpected async error:

it('should not render fixed ad', function* () {
  const ampIframe = createAmpIframe(env, { src: iframeSrc, sandbox: 'allow-scripts allow-same-origin', width: 300, height: 250, position: 'fixed' }, 0);
  yield whenUpgradedToCustomElement(ampIframe);
  yield ampIframe.signals().whenSignal(CommonSignals.LOAD_START);
});

Error message printed by the test:

ERROR: 'amp-iframe is not used for displaying fixed ad. Please use amp-sticky-ad and amp-ad instead.'
    The test "amp-iframe   amp-iframe should not render fixed ad" resulted in a call to console.error. (See above line.)
    ⤷ If the error is not expected, fix the code that generated the error.
    ⤷ If the error is expected (and synchronous), use the following pattern to wrap the test code that generated the error:
        'allowConsoleError(() => { <code that generated the error> });'
    ⤷ If the error is expected (and asynchronous), use the following pattern at the top of the test:
        'expectAsyncConsoleError(<string or regex>[, <number of times the error message repeats>]);'

The same test, but it now expects the error:

it('should not render fixed ad', function* () {
  expectAsyncConsoleError(/not used for displaying fixed ad/);
  const ampIframe = createAmpIframe(env, { src: iframeSrc, sandbox: 'allow-scripts allow-same-origin', width: 300, height: 250, position: 'fixed' }, 0);
  yield whenUpgradedToCustomElement(ampIframe);
  yield ampIframe.signals().whenSignal(CommonSignals.LOAD_START);
});

Fixes #15658
Addresses #16916 (review) and #17209 (review)
Closes #14406
Partially fixes #14360

[rsimha]

/to @choumx @danielrozenberg
/cc @cramforce @jpettitt @lannka @zhouyx @prateekbh @aghassemi @alanorozco @cvializ @gmajoulet

[danielrozenberg]

Approval for code; let others discuss the policy change itself

[danielrozenberg]

This TODO is about the next line, I assume. Are you removing it on purpose as well?

[rsimha]

Yes. We want to continue capturing console errors.

[danielrozenberg]

🙃

[cramforce]

Would it be possible to add one instance to the PR where such an async error is correctly "expected" by a test.

[codecov-io]

Codecov Report

Merging #17213 into master will increase coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #17213      +/-   ##
==========================================
+ Coverage   77.78%   77.87%   +0.09%     
==========================================
  Files         562      562              
  Lines       41164    41164              
==========================================
+ Hits        32018    32057      +39     
+ Misses       9146     9107      -39

Flag	Coverage Δ
#integration_tests	36.17% <ø> (+0.03%)	⬆️
#unit_tests	76.93% <ø> (+0.08%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30cc384...48b111b. Read the comment docs.

[rsimha]

@cramforce Done. Added an example to the PR description.

[lannka]

Agree that we should avoid throwing on error messages, which might hit the Mocha bug.

Is there a way to fail travis at the end if we see unexpected console errors?