Bring calledWith messages more in line with Sinon's output #152
Conversation
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
changed the title
cincodenada
changed the title
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
force-pushed
the
better-diffing-still
branch
from
34ae581
to
e3fb6e0
Compare
|
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 Overall I think this is a reasonable solution and looks pretty decent, but I'm happy to hear feedback on style or approach here. |
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
%Dfor the nonNegated case, which eliminates the noisy duplication, while still using%*%Cfor the negated case. This matchesassert.jsin core Sinon as best I can tell.As shorthand (and to support other existing code), I supported specifying a single value for
suffixesinstead, 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: