🏗 Use a mock instead of a stub to catch console errors during tests #14432

rsimha · 2018-04-05T00:41:30Z

In #14336, a stub was added for console.error that would throw an exception when called from within a test.

This PR implements the same check more elegantly by using a mock for console.error. It also adds a helper function called allowConsoleError that tests can use when they want execute code that may generate an error.

For now, given the sheer number of AMP tests that generate console errors, we print a warning after each test (local dev) / after all tests (on Travis) with instructions for how to fix these errors. Once all of them are fixed (or wrapped in an allowConsoleError block), we can start failing tests when an error is encountered.

Examples:

This test will print a warning...

it('test with unexpected console errors', () => {
  doAThing();  // No console error
  doAnotherThing(); // Contains a console error
  doYetAnotherThing(); // Contains a console error
});

... and so will this test...

it('test with some unexpected console errors', () => {
  doAThing();  // No console error
  allowConsoleError(() => {
    doAnotherThing(); // Contains a console error
  });
  doYetAnotherThing(); // Contains a console error
});

... but this test will pass...

it('test with all expected console errors', () => {
  doAThing();  // No console error
  allowConsoleError(() => {
    doAnotherThing(); // Contains a console error
    doYetAnotherThing(); // Contains a console error
  });
});

Fixes #7381
Follow up to #14336
Related to #14406

rsimha · 2018-04-05T00:43:10Z

/to @cramforce @choumx @cramforce

rsimha · 2018-04-05T01:43:42Z

This is incomplete as is, and needs an override for the chai assertion expect().to.throw.

rsimha · 2018-04-05T11:41:18Z

@choumx After further debugging, it appears that using a mock will break existing usage of expect().to.throw in our tests.

~~I suspect we're going to have to abandon this PR and go back to what @danielrozenberg was doing with the stub.~~

Will discuss in person to see if there's a way around this with mocks.

Edit: Found a way to use mocks.

danielrozenberg

How many tests are being broken by this? If it's not a lot we should change those tests to use this mock behaviour instead of abandoning this approach

I seriously think that throwing an exception from calls to console.error(…) is the wrong way to go, it breaks the expectations from the API (I think of JavaScript's console.error(…) in the same way I think of Python 3's print(…, file=sys.stderr). It prints to the console, that's all I expect of it to do. Libs I use will use it without my control)

danielrozenberg · 2018-04-05T15:38:48Z

test/_init_tests.js

+        '  ⤷ Call "this.consoleMock.expects(\'error\').withArgs' +
+            '(\'<message>\')" before the code containing the error.\n' +
+        '  ⤷ Call "this.consoleMock.expects(\'error\').never()" ' +
+            'after the code containing the error.\n';


I'm not sure if you have add the expectation before the offending call to console.error(…), since mocks are only verified in the afterEach hook. You can remove the "before the code containing the error" part, replace it with "inside your test". Test writers should be able to decide where they want to write the expectations :)

Does this .never() resetting work as expected? Again, since .verify() happens at the end, I think you're telling test writers to reset the expectation that they just wrote, before the mock gets verified. I think if you want the test to be very explicit it should be something like:

… this.consoleMock.expects('error').withArgs('foo', 'bar'); doStuff(); // calls console.error('foo', 'bar'); this.consoleMock.verify(); this.consoleMock.expects('error').never(); …

But this seems a bit too explicit. Maybe instead we can suggest this:

this.consoleMock.expects('error').once().withArgs('foo', 'bar');

(this way, the .verify() call at the end verifies the .once() constraint)

<message> is vague. I suggest adding a link to http://sinonjs.org/releases/v4.5.0/matchers/ (nit: can we dynamically replace the version number to always point to the Sinon version we're using?)

Based on offline discussions and debugging with @danielrozenberg, I retract my comment above re: abandoning the approach that uses mocks. Updated commits coming up. The above comments are moot.

rsimha · 2018-04-05T16:10:40Z

Responding to top level comment re: mocks vs stubs first.

There are 541 instances of to.throw() in our tests, and a sizeable number of them look for an error message.

https://gist.github.com/rsimha/0e2b456f5d41d953692840380f65fbe1

danielrozenberg · 2018-04-05T16:18:04Z

A cursory glance over a few of these seems to show that these errors are thrown using user().assert() or dev().assert(). Shouldn't that still work then?

rsimha · 2018-04-05T16:25:08Z

I believe user.assert() and dev().assert internally call console.log(). There is likely some logic that causes them to do things differently in non-local-dev or non-test mode, but I'm not sure what it is.

Edit: Here's where src/log.js uses console.error: https://github.com/ampproject/amphtml/blob/master/src/log.js#L153

danielrozenberg · 2018-04-09T15:08:34Z

build-system/tasks/karma.conf.js

@@ -197,7 +197,7 @@ module.exports = {
      // Longer timeout on Travis; fail quickly at local.
      timeout: process.env.TRAVIS ? 10000 : 2000,
    },
-    captureConsole: false,
+    captureConsole: true, // DEBUGGING


I'm assuming this is supposed to be gone before merging?

danielrozenberg · 2018-04-09T15:13:55Z

test/_init_tests.js

+    consoleMock.verify();
+  } catch (e) {
+    const helpMessage =
+        'Your test resulted in an unexpected call to console.error.\n' +


nit: :%s/unexpected call/uncaught call/g (since you're continuing later to say "if this was expected/not expected, it seems odd to announce from beforehand that the call was unexpected)

Removed the word unexpected since the error message from the mock says so anyway.

danielrozenberg · 2018-04-09T15:15:34Z

test/_init_tests.js

@@ -265,13 +266,40 @@ sinon.sandbox.create = function(config) {
  return sandbox;
 };

+function mockConsoleError() {


nit: the naming of mockConsoleError and forbidConsoleError bother me, since the "forbidding" part is happening inside the function called "mock". Not sure how I would rename them tho...

rsimha

@danielrozenberg PTAL. Due to the sheer number of tests that need changing, I've changed this to warning mode for now.

During local dev, if a test contains unexpected console errors, a message will be printed immediately after the failure. On Travis, all console errors and the accompanying messages will be printed at the end.

Note that we don't fail tests yet, but plan on doing so in future.

rsimha · 2018-04-10T21:05:55Z

test/_init_tests.js

+    consoleMock.verify();
+  } catch (e) {
+    const helpMessage =
+        'Your test resulted in an unexpected call to console.error.\n' +


Removed the word unexpected since the error message from the mock says so anyway.

rsimha · 2018-04-10T21:08:11Z

test/_init_tests.js

@@ -265,13 +266,40 @@ sinon.sandbox.create = function(config) {
  return sandbox;
 };

+function mockConsoleError() {


…onsole.error may be called

* Fix sanitizer regex * handlbars to indiclate templated hotpatch bar * css fix on example * New test pattern per PR #14432 * fixes per pr comments

…14432)

* Fix sanitizer regex * handlbars to indiclate templated hotpatch bar * css fix on example * New test pattern per PR #14432 * fixes per pr comments

rsimha added the WG: infra label Apr 5, 2018

rsimha self-assigned this Apr 5, 2018

rsimha requested review from danielrozenberg and dreamofabear April 5, 2018 00:41

googlebot added the cla: yes label Apr 5, 2018

rsimha mentioned this pull request Apr 5, 2018

✅ Replace the console.error stub with a mock in _init_tests.js #14429

Closed

rsimha requested a review from cramforce April 5, 2018 00:42

dreamofabear approved these changes Apr 5, 2018

View reviewed changes

danielrozenberg requested changes Apr 5, 2018

View reviewed changes

rsimha mentioned this pull request Apr 6, 2018

Change tests to fail on logged errors unless they are expected #7381

Closed

danielrozenberg approved these changes Apr 9, 2018

View reviewed changes

rsimha commented Apr 10, 2018

View reviewed changes

rsimha added 4 commits April 10, 2018 22:44

Use a mock instead of a stub to catch console errors during tests

b9bb917

Move console error mocks into helper functions

d294b02

Add allowConsoleError(func) which lets you run test code from which c…

1d9fdf3

…onsole.error may be called

Fix all tests that obviously expect an error

2e39119

danielrozenberg approved these changes Apr 11, 2018

View reviewed changes

rsimha merged commit 18520b4 into ampproject:master Apr 11, 2018

rsimha deleted the 2018-04-04-ErrorMock branch April 11, 2018 15:33

This was referenced Apr 11, 2018

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

Closed

✅ Unskip tests that were skipped due to console errors #14549

Merged

jpettitt added a commit to jpettitt/amphtml that referenced this pull request Apr 12, 2018

New test pattern per PR ampproject#14432

f9bf4f1

erwinmombay pushed a commit that referenced this pull request Apr 12, 2018

✨ amp-geo pre-release testing (#14577)

8f6d46e

* Fix sanitizer regex * handlbars to indiclate templated hotpatch bar * css fix on example * New test pattern per PR #14432 * fixes per pr comments

danielrozenberg mentioned this pull request Apr 12, 2018

✅ Wrap tests in test-amp-mustache.js that expect a console error with allowConsoleError #14588

Merged

danielrozenberg mentioned this pull request Apr 12, 2018

Wrap a single test in test-amp-form.js that expect a console error with allowConsoleError #14592

Closed

glevitzky pushed a commit that referenced this pull request Apr 27, 2018

🏗 Use a mock instead of a stub to catch console errors during tests (#…

29ac284

…14432)

glevitzky pushed a commit that referenced this pull request Apr 27, 2018

✨ amp-geo pre-release testing (#14577)

d5317fc

* Fix sanitizer regex * handlbars to indiclate templated hotpatch bar * css fix on example * New test pattern per PR #14432 * fixes per pr comments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🏗 Use a mock instead of a stub to catch console errors during tests #14432

🏗 Use a mock instead of a stub to catch console errors during tests #14432

rsimha commented Apr 5, 2018 •

edited

Loading

rsimha commented Apr 5, 2018

rsimha commented Apr 5, 2018

rsimha commented Apr 5, 2018 •

edited

Loading

danielrozenberg left a comment

danielrozenberg Apr 5, 2018

rsimha Apr 5, 2018

rsimha commented Apr 5, 2018

danielrozenberg commented Apr 5, 2018

rsimha commented Apr 5, 2018 •

edited

Loading

danielrozenberg Apr 9, 2018

danielrozenberg Apr 9, 2018

rsimha Apr 10, 2018

danielrozenberg Apr 9, 2018

rsimha Apr 10, 2018

rsimha left a comment •

edited

Loading

rsimha Apr 10, 2018

rsimha Apr 10, 2018

🏗 Use a mock instead of a stub to catch console errors during tests #14432

🏗 Use a mock instead of a stub to catch console errors during tests #14432

Conversation

rsimha commented Apr 5, 2018 • edited Loading

rsimha commented Apr 5, 2018

rsimha commented Apr 5, 2018

rsimha commented Apr 5, 2018 • edited Loading

danielrozenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsimha commented Apr 5, 2018

danielrozenberg commented Apr 5, 2018

rsimha commented Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsimha left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsimha commented Apr 5, 2018 •

edited

Loading

rsimha commented Apr 5, 2018 •

edited

Loading

rsimha commented Apr 5, 2018 •

edited

Loading

rsimha left a comment •

edited

Loading