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

Improve error message format for Node's assert.fail #9262

Merged
merged 3 commits into from
Dec 7, 2019

Conversation

wsmd
Copy link
Contributor

@wsmd wsmd commented Dec 5, 2019

Summary

This PR fixes incorrect error message format when using Node's assert.fail.

Currently when using assert.fail in a test, We get a message that includes two parts that should be omitted:

  • Expected/Received with undefineds
  • An empty "Difference".

This was fixed by updating the node assert message helpers in both of jest-jasmine2 and jest-circus to account for the fail operator.

Before

✕ does not print expected/received message for assert.fail (1ms)

● does not print expected/received message for assert.fail

  assert.fail(received, expected)

  Expected value fail to:
    undefined
  Received:
    undefined
  
  Message:
    nooooooo

  Difference:

  Compared values have no visual difference.

    2 | 
    3 | it('does not print expected/received message for assert.fail', () => {
  > 4 |   assert.fail('nooooooo');
      |          ^
    5 | })

    at Object.fail (test.js:4:10)

After

✕ does not print expected/received message for assert.fail (3ms)

● does not print expected/received message for assert.fail

  assert.fail(received, expected)

  Message:
    nooooooo

    2 | 
    3 | it('does not print expected/received message for assert.fail', () => {
  > 4 |   assert.fail('nooooooo');
      |          ^
    5 | })

    at Object.fail (test.js:4:10)

Test plan

Added two new tests to make sure the error message printed for assert.fail looks as expected.

Comment on lines +79 to +81
if (stack.match('.fail')) {
return 'fail';
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This stack.match fallback is only needed for Node versions older than v10.11 because it wan't until v10.11.0 that the AssertionError of assert.fail had an operator value set by default (nodejs/node#22694).

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe mention that in a comment, it's common to grep the codebase for things like node 10 when phasing out support for an EOL version

@jeysal jeysal requested a review from pedrottimark December 5, 2019 11:47
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Missing changelog entry, but the code looks great. Thanks!

@wsmd wsmd changed the title Fix incorrect error message format for Node's assert.fail Improve error message format for Node's assert.fail Dec 5, 2019
@SimenB SimenB merged commit f4940ae into jestjs:master Dec 7, 2019
@SimenB
Copy link
Member

SimenB commented Dec 7, 2019

Thank you!

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants