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

Update error message on assert when representation of expected and ac… #2303

Merged
merged 1 commit into from Oct 13, 2020
Merged

Update error message on assert when representation of expected and ac… #2303

merged 1 commit into from Oct 13, 2020

Conversation

romanbalayan
Copy link
Contributor

Purpose (TL;DR) - mandatory

Fixes issue #2084 by adding quotes to diff values if said values are strings, creating distinct string representations and preventing the expected and actual values from being reduced to a single value.

Background (Problem in detail) - optional

When the expected and actual values of an assertion do not match but they have the same string representation (for example the number 1234 and the string "1234"), the output does not show the difference, but reduces it to a single value. In this example, this output is produced:

expected spy to be called with arguments
1234
while this output was expected:

expected spy to be called with arguments
1234
1234

Solution - optional

A solution was proposed in the ticket itself which was also implemented here: adding quotes around a diff value if the value in question is a string. This process is skipped if the value is undefined, not a string, or already wrapped in quotes.

Instead, diffs are now displayed as follows, given the example where the string 1234 was expected but the number 1234 was found instead:

expected spy to be called with arguments
1234
'1234'

How to verify - mandatory

  1. Check out this branch
  2. npm install
  3. Run the test included with this PR

Checklist for author

  • npm run lint passes
  • References to standard library functions are cached.

@fatso83 fatso83 added the semver:major changes will cause a new major version label Oct 13, 2020
@fatso83
Copy link
Contributor

fatso83 commented Oct 13, 2020

@mroderick Is this a semver major change? If someone parses sinon's output and expects it to be a certain format, I guess this would break it. So maybe just bump the major ...

@romanbalayan Thanks for the fix!

@fatso83 fatso83 added semver:minor changes will cause a new minor version and removed semver:major changes will cause a new major version labels Oct 13, 2020
@fatso83 fatso83 merged commit 3b1fa42 into sinonjs:master Oct 13, 2020
@romanbalayan
Copy link
Contributor Author

Hi @fatso83,

Also adding the same request here, hopefully it would not be much of a bother for you to add the hacktoberfest-accepted label to this PR so it may be counted towards my progress?

Thank you!

@mroderick
Copy link
Member

This has been published with sinon@9.2.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted semver:minor changes will cause a new minor version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants