-
Notifications
You must be signed in to change notification settings - Fork 107
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
Add calledOnceWith and calledOnceWithExactly #117
Conversation
lib/sinon-chai.js
Outdated
sinonMethod("calledWithExactly", "been called with exact arguments %*", "%D"); | ||
sinonMethod("calledOnceWithExactly", "been called exactly once with exact arguments %*", "%C"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, here's the thing about these, specifically their nonNegatedSuffix
es. There's really no good way to address these new ones.
Using the ", but it was called %c%C"
suffix doesn't make sense when it's only been called once. And using "%D"
causes some odd formatting issues in the case of multiple calls.
With "%D"
, it ends up looking like this:
AssertionError: expected spy to have been called with exact arguments A, BCall 1:
C A
A B
Call 2:
C A
B
I'm about 99.999% sure this is a sinon formatter issue and not an issue with sinon-chai.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be resolved by this ticket: sinonjs/sinon#1717
I'm still open to suggestions on how these nonNegatedSuffix
es should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just using "%D"
should work pretty well, after that Sinon bug is fixed at least. Do you agree?
package.json
Outdated
"chai": "^4.1.0", | ||
"eslint": "^3.19.0", | ||
"chai": "^4.1.2", | ||
"eslint": "^4.18.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave dev dependency updates for a separate PR, and only update Sinon in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a few messages tests too?
LGTM, but can you regenerate package-lock.json? I don't think you rolled back the changes there. |
Good call. I did it all via Github. |
What do you mean by "messages tests"? As in testing the formatting of the failure messages? |
Yeah, see messages.js. Historically we've had a lot of regressions there (e.g. via Sinon upgrades that make our current messages nonsensical) that those tests have caught. |
I'll take a crack at that in the next couple of days when I get time. Thanks for reviewing this. |
@domenic Just added some messages tests. Can you please take another look? |
Sorry for the massive delay here :(. This will go out in a release today! |
This should resolve #116.