Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

danielrozenberg
Copy link
Member

Followup to PR #14406

Note that to fix skipped tests now:

This test will fail...

it('test with a console error', () => {
  console.error('foo');
});

... but this test will pass...

it('test with an expected console error', () => {
  sinon.mock(console).expects('error').withArgs('foo');
  console.error('foo');
});

You can also match multiple arguments and RegExp with:

it('test with an expected console error', () => {
  sinon.mock(console).expects('error').withArgs(
      'foo', sinon.match(/ba./)); // or withExactArgs to match all args
  console.error('foo', 'bar', 'baz');
});

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@@ -292,7 +290,7 @@ function beforeTest() {
// Global cleanup of tags added during tests. Cool to add more
// to selector.
afterEach(function() {
consoleSandbox.restore();
consoleSandbox.verify();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please wrap this in a try block and in the catch augment the error message with instructions for how to add an expectation.

consoleSandbox.stub(console, 'error').callsFake((...messages) => {
throw new Error(messages.join(' '));
});
consoleSandbox = sinon.mock(console);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that tests will have to update the expectations in this mock if they want to allow error logging, maybe you can get rid of the local var at the top of this file and update this.consoleSandbox? Then tests can do:

this.consoleSandbox.expects('error').withArgs('foo');

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, for readability, a better name might be this.consoleMock.

@rsimha
Copy link
Contributor

rsimha commented Apr 5, 2018

This is now being done in #14432. All comments addressed there. Closing this.

@rsimha rsimha closed this Apr 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants