-
Notifications
You must be signed in to change notification settings - Fork 622
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
feat(testing): Add mocking utilities #2048
Conversation
testing/mock/_asserts_test.ts
Outdated
); | ||
|
||
spyFunc(); | ||
assertSpyCall(spyFunc, 0); |
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.
Is this call signature useful? It doesn't seems obvious to me that what is asserted with this call
How about making the 3rd argument non-optional?
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.
In assertSpyCall
, the first check it does is that it verifies there were enough calls for the index passed in to exist. By calling it without a third arg or with empty object for the third arg, you are basically asking it to assert it was called enough times for spyFunc.calls[index]
to exist and it will tell you how many times it was actually called if it doesn't.
Below is an example demonstrating how this is useful over just accessing the calls directly on the spyFunc
. assertSpyCall
uses assertEquals
for it's arguments. If you wanted to do a strict equals comparison on one of the arguments, you would need to get the call and make the assertion on the arg like shown below.
const call = spyFunc.calls[0];
assertStrictEquals(call.args[0], thing);
The problem with this is that if spyFunc
wasn't called as many times as expected, call will be undefined and it would throw because args
doesn't exist on undefined. You could add optional chaining to avoid it throwing that way but then it would throw an error where you don't know if it's because call?.args[0]
was undefined or if the spyFunc
just wasn't called that many times.
const call = assertSpyCall(spyFunc, 0);
assertStrictEquals(call.args[0], thing);
With assertSpyCall
, if spyFunc wasn't called enough times it would give you a clear error saying how much it was actually called and if it was called enough times you can confidently know that if the next assertion fails because call.args[0]
is undefined, you would know the function was called incorrectly.
In the above example, the first line could also have other assertions about the call such as what this
was or what it returned when it was called.
testing/mock/_asserts.ts
Outdated
|
||
/** | ||
* Asserts that a spy is called as expected. | ||
* Returns the call. |
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 returning value from assertion is confusing and less readable
(Note: We had similar discussion in the past about this design #1052 (comment)
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.
We also can get n-th call object from spy.calls[n]. I think there's no need of returning call object here
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.
Would you be good with keeping the third argument optional so that it can still be used for just checking if the spyFunc was called enough times but just having this assertion function changed to not returning a value?
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 went ahead and made this change, the assert functions no longer return anything.
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.
Thanks!
An example of these mocking utilities being used can be found in my other PR. Here is a link to that test file. I'll write up a section about how to use mock for the README.md file tomorrow. |
I decided against this. My last commit just moves the files from the testing/mock directory into the testing directory. Any other mocking utilities that are made, like I'll add a section to the |
I could move the |
I decided to also go ahead with this change. I thought there being both an asserts.ts and _asserts.ts file might cause some confusion. |
I've added a section to the README.md file that explains why and how to use the |
|
||
Say we have two functions, `randomMultiple` and `randomInt`, if we want to | ||
assert that `randomInt` is called during execution of `randomMultiple` we need a | ||
way to spy on the `randomInt` function. That could be done with either either of |
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.
nit:
way to spy on the `randomInt` function. That could be done with either either of | |
way to spy on the `randomInt` function. That could be done with either of |
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 was a mistake I missed when typing this up. I can fix it tonight after work.
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 sentence wasn't fixed before this PR was merged. I'll fix this tonight in one of my PRs that haven't been merged yet that depend on this.
README section looks great to me! The purpose of spy and stub are explained very well with examples 👍 |
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.
Type friendly definitions of spy and stub look really impressive to me. Great 👍 Tests are also written very thoroughly. Nice work!
LGTM
One last bikeshedding point: Mock related assertions ( |
When it comes to using mock, I just see it as a test utility. Nobody would have a reason to use the mock assertions without also using mock. That's why I thought it would be best to keep them together instead of putting the spy assertions into asserts.ts. We could move those functions and MockError to asserts.ts though if that would be preferred. |
Fair enough. Ok. Let's keep it as is |
To be able to add my test_suite module for getting
describe
andit
like functions into Deno, I need a way to mockDeno.test
andt.step
to verify that the tests get registered correctly. I could potentially re-write the tests to do snapshot testing but that would require a lot more test coverage to verify all of the behavior handled byDeno.test
andt.step
are handled correctly when called bydescribe
orit
.The issue for adding the test_suite module to std can be found here.
I created
mock.ts
for generalized mocking utilities andmod.ts
for exporting all mocking utilities. I plan on later trying to create a PR to addtime.ts
from the mock module as a tool for people to be able to fake time for their tests. I thought about having it at the top level but thought it might cause confusion later on having it there. In addition to that, not everyone that will useasserts.ts
will want to make use of the mock utilities, so I put it in its own sub folder.deno docs
The mock module was inspired by sinon.js which is a common testing library.
I haven't written a
README.md
file yet for mock. I figured it would be best to get some feedback first before writing that. Here is the latestREADME.md
for the mock module. I'm thinking of just copying over the spy and stub usage sections tomock/README.md
.