-
-
Notifications
You must be signed in to change notification settings - Fork 771
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
Asserting on callCount with a numeric string results in very confusing failure message #2408
Labels
Comments
Great bug report. We always lean on the side of being correct and throwing early rather than try and coerce types, so any fix should take that into account. |
cincodenada
added a commit
to cincodenada/sinon
that referenced
this issue
Nov 3, 2021
This is to fixes sinonjs#2408, which could result in error messages like "expected spy to be called 10 times but was called 10 times". Now we will instead say "expected '10' to be a number, but was of type string", which is much clearer!
cincodenada
added a commit
to cincodenada/sinon
that referenced
this issue
Nov 5, 2021
…e of callCount argument and error accordingly This is to fixes sinonjs#2408, which could result in error messages like "expected spy to be called 10 times but was called 10 times". Now we will instead say "expected '10' to be a number, but was of type string", which is much clearer!
cincodenada
added a commit
to cincodenada/sinon
that referenced
this issue
Nov 5, 2021
…ror accordingly This is to fixes sinonjs#2408, which could result in error messages like "expected spy to be called 10 times but was called 10 times". Now we will instead say "expected '10' to be a number, but was of type string", which is much clearer!
cincodenada
added a commit
to cincodenada/sinon
that referenced
this issue
Nov 5, 2021
…ror accordingly This is to fixes sinonjs#2408, which could result in error messages like "expected spy to be called 10 times but was called 10 times". Now we will instead say "expected '10' to be a number, but was of type string", which is much clearer!
cincodenada
added a commit
to cincodenada/sinon
that referenced
this issue
Nov 5, 2021
…ror accordingly This is to fixes sinonjs#2408, which could result in error messages like "expected spy to be called 10 times but was called 10 times". Now we will instead say "expected '10' to be a number, but was of type string", which is much clearer!
cincodenada
added a commit
to cincodenada/sinon
that referenced
this issue
Nov 5, 2021
…ingly This is to fixes sinonjs#2408, which could result in error messages like "expected spy to be called 10 times but was called 10 times". Now we will instead say "expected '10' to be a number, but was of type string", which is much clearer!
cincodenada
added a commit
to cincodenada/sinon
that referenced
this issue
Nov 5, 2021
…ingly This is to fixes sinonjs#2408, which could result in error messages like "expected spy to be called 10 times but was called 10 times". Now we will instead say "expected '10' to be a number, but was of type string", which is much clearer!
cincodenada
added a commit
to cincodenada/sinon
that referenced
this issue
Nov 5, 2021
…ingly This is to fixes sinonjs#2408, which could result in error messages like "expected spy to be called 10 times but was called 10 times". Now we will instead say "expected '10' to be a number, but was of type string", which is much clearer!
cincodenada
added a commit
to cincodenada/sinon
that referenced
this issue
Nov 5, 2021
This is to fixes sinonjs#2408, which could result in error messages like "expected spy to be called 10 times but was called 10 times". Now we will instead say "expected '10' to be a number, but was of type string", which is much clearer!
cincodenada
added a commit
to cincodenada/sinon
that referenced
this issue
Nov 5, 2021
This is to fixes sinonjs#2408, which could result in error messages like "expected spy to be called 10 times but was called 10 times". Now we will instead say "expected '10' to be a number, but was of type string", which is much clearer!
fatso83
pushed a commit
that referenced
this issue
Jan 20, 2022
* Strip stack frames in `this.message` This saves us from having to do it every time, and makes things much nicer. Also use a little bit more specific regex, to avoid issues with messages that happen to contain the word "at" * Check type of callCount argument and error accordingly This is to fixes #2408, which could result in error messages like "expected spy to be called 10 times but was called 10 times". Now we will instead say "expected '10' to be a number, but was of type string", which is much clearer! * A little more explanatory comment * Edit the comment about appending stack frames What's actually happening here is that we want to add a frame of context to `callStr`, but the first two stack frames will be within Sinon code and thus probably not helpful to the end-user. So, we skip the first two stack frames, and append the third stack frame, which should contain a meaningful location to the end-user. * Add test for adding stack traces to error message This ensures that if at some point we end up with another Sinon layer in the stack at some point, we'll catch it and hopefully adjust accordingly For reference, as of this commit, the Sinon portion of the stack is: lib/sinon/proxy-invoke.js:65:15 lib/sinon/proxy.js:265:26 Also convert a neighboring test to async while we're at it
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
This bug was originally reported in
sinon-chai
by @BebeSparkelSparkel as chaijs/sinon-chai#136 but turned out to be an issue with Sinon itself. The bug is that we can get the error messageAssertError: expected spy to be called 10 times but was called 10 times
- this happens when specifyingcallCount
as a string.To Reproduce
I made a slightly modified version of the original test case so that it just uses Sinon:
Run the above, you will get
AssertError: expected spy to be called 10 times but was called 10 times
which is a very confusing error message.Expected behavior
One of two options:
Both of these seem sensible, the first is a little more user-friendly, the second is a little more correct.
Internally it looks like Sinon raises a
TypeError
, which is I think what is actually triggering this failure, so it could just surface that, or it could attempt to convert to a number.This actually happens with any string:
sinon.assert.callCount(s, 'banana')
will get youexpected spy to be called banana times but was called 10 times
, which is less confusing, but also it would probably be better to instead fail with an error aboutbanana
not being a number.Context (please complete the following information):
The text was updated successfully, but these errors were encountered: