-
-
Notifications
You must be signed in to change notification settings - Fork 769
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 a calledOnceWithMatch assertion #2294
Conversation
Thank you for the pull request! I agree, it seems silly not to have I'm in favour of duplication in tests, as that makes each set easy to understand (and throw out) ... as opposed to really DRY tests, that require a lot of refactoring every time there's an expansion of the API. Please go ahead with the tests 🚀 @mantoni we should synchronise the list of assertions in |
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 have a need for this sometimes as well. Thank you! 🙏👍
I've added testing based on calledOnceWithExactly, which at least confirms it works. I changed one slightly to specifically test the "match" part. I'm happy for this to be merged 👍 |
@richthegeek Regarding your suggestion to compose assertions instead of adding too much suggar, I'd like to highlight that this is a possible alternative that works today: const spy = sinon.spy();
spy('some test');
sinon.assert.calledWith(spy, sinon.match('test')); |
@matoni great 👍 I meant mostly that having |
Released in |
Purpose
This adds an assertion which combines
calledOnce
andcalledWithMatch
intocalledOnceWithMatch
.Background (Problem in detail)
This is a common pattern in tests that I write:
It would be great to only need write:
It seems like uneven coverage to combine
calledOnce + calledWithExactly
whilst not covering this.However, I can see that there is a potential for temptation to add too much sugar - no-one wants a combinatorial explosion! Many libraries solve this with something like
sinon.assert.calledOnce(stub).withMatch(arg1, arg2)
but that's not a discussion for this PR!Testing
I was looking through the test files and wasn't entirely sure what coverage would be appropriate here - I'm happy to essentially duplicate the
calledOnceWithMatch
testing if that is sufficient coverage and validation, but it feels incomplete.Please let me know what you think.
How to verify - mandatory
npm install
npm test
Checklist for author
npm run lint
passes