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

Bring calledWith messages more in line with Sinon's output #152

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

cincodenada
Copy link
Contributor

@cincodenada cincodenada commented Nov 1, 2021

This is a continuation in the spirit of #104, but resolves the awkwardness with there being unnecessary verbosity in the negative versions of the error messages where we list the arguments twice, which brings us in line with the Sinon core messages again.

To do this, I changed the nonNegatedSuffix parameter to instead take a pair of suffixes, nonNegated and negated. This allows us to specify %D for the nonNegated case, which eliminates the noisy duplication, while still using %*%C for the negated case. This matches assert.js in core Sinon as best I can tell.

As shorthand (and to support other existing code), I supported specifying a single value for suffixes instead, which I use for nonNegatedSuffix and assume an empty negatedSuffix, which makes it backwards-compatible.

This caused the tests to get a little hairier, because we can no longer depend on the simple list of arguments in the positive cases. I think I wrangled them into fairly reasonable form, but I'm happy to hear other ideas there.

Other things I tried or considered for the tests:

  • Disable color output (only possible by manipulating process.env or process.argv, both of which seem like a no-go)
  • Create a custom matcher that strips the message before comparing (throw() doesn't support matchers)
  • Matching color escapes with a RegExp (seemed way messier than this)
  • Manually try/catch, store error, assert against the error (also seemed messier than this)

This is a continuation in the spirit of chaijs#104, but resolves the
awkwardness with the negative version, and brings us closer to core
Sinon messages.

To do this, we changed the nonNegatedSuffix parameter to instead take a
pair of suffixes, nonNegated and negated.  This allows us to specify %D
for the nonNegated case, which eliminates noisy duplication, while still
using %*%C for the negated case. This also matches what core Sinon
outputs in these cases.

As shorthand, we still support specifying a single value, which we
use for nonNegatedSuffix and assume an empty negatedSuffix.
I'm moderately happy with this. It got a little messy because now we
can't depend on the brief but duplicative listing of arguments, and have
to match against the diff view, which is colorized and has newlines and
things.

I pulled in strip-ansi@6 because it's already a transitive dependency,
and added a little wrapper function to rewrite the error messages being
thrown.

The wrapper is a little weird, but was the best solution I came up with.
Other things I tried or considered:
 - Disable color output (only possible by manipulating process.env or
   process.argv, both of which seem like a no-go)
 - Create a custom matcher that strips the message before comparing
   (throw() doesn't support matchers)
 - Matching color escapes with a RegExp (seemed way messier than this)
 - Manually try/catch, store error, assert against the error (also
   seemed messier than this)

The wrapper could be elsewhere too - it could be a wrapper that calls
expect(), for instance - but I think this is clear enough.
@cincodenada cincodenada changed the title Further improve spy diffs Bring calledWith messages in line with Sinon again Nov 1, 2021
@cincodenada cincodenada changed the title Bring calledWith messages in line with Sinon again Bring calledWith messages more in line with Sinon's output Nov 1, 2021
This was broken before this PR, but might as well fix it
I discovered this bug while working on this, there's a PR for fix here
for reference: sinonjs/sinon#2407
This is a bit of a regression imo on Sinon's part, but I don't see an
easy way to adjust for it
@cincodenada
Copy link
Contributor Author

cincodenada commented Nov 2, 2021

Okay, I was feeling ambitious so I went ahead and fixed all the pipeline errors, whether they were related to my changes or not.

This required some more complex tests - basically, more regexes, and stripping some things out of the error messages. I've added helper methods to generate some of the regexes, and ended up adding a wrapper similar to the color-stripping one to strip quotes, since newer versions of Sinon (9/10) quote things like called exactly 'once' or expected to throw 'TypeError' where earlier versions don't. I initially solved that with more RegExps, but then I hit a bug in Sinon (sinonjs/sinon#2407) which resulted in an absurd level of recursive escaping, so I went with stripping to deal with that. I also put in a pull request to fix it in Sinon, but in the meantime this is a fine workaround, I think.

Overall I think this is a reasonable solution and looks pretty decent, but I'm happy to hear feedback on style or approach here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant