-
Notifications
You must be signed in to change notification settings - Fork 7
Add ignoreExtraArgs option
#36
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
Conversation
mcous
left a 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.
This looks great! I think there's a little bug in the arity check logic to fix. Also, I think it's really important that the calledWith types are updated to reflect ignoreExtraArgs: true, e.g.
const spy = vi.fn<(a: string, b: number) => boolean>()
// all these should pass type-checking
when(spy, { ignoreExtraArgs: true }).calledWith()
when(spy, { ignoreExtraArgs: true }).calledWith('hello')
when(spy, { ignoreExtraArgs: true }).calledWith('hello', 123)This requires a bit of relatively involved typescript, so if it blows up the diff or anything, just let me know and I can take care of it
src/behaviors.ts
Outdated
| // Check arity | ||
| const expectedArguments = behavior.args | ||
| const { ignoreExtraArgs } = behavior | ||
| if (expectedArguments.length !== actualArguments.length && !ignoreExtraArgs) | ||
| return false | ||
|
|
||
| // Check arguments | ||
| return expectedArguments.every((expectedArgument, index) => { | ||
| return equals(actualArguments[index], expectedArgument) | ||
| }) |
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 like this arity check before the arguments - it makes me realize there's a subtle bug in the current implementation around explicit vs implicit undefined values
A few suggestions, in priority order
- Should the check be
>in theignoreExtraArgscase? - I don't think these comments add any extra value to the code, let's lose them in favor of variable names
| // Check arity | |
| const expectedArguments = behavior.args | |
| const { ignoreExtraArgs } = behavior | |
| if (expectedArguments.length !== actualArguments.length && !ignoreExtraArgs) | |
| return false | |
| // Check arguments | |
| return expectedArguments.every((expectedArgument, index) => { | |
| return equals(actualArguments[index], expectedArgument) | |
| }) | |
| const { args: expectedArguments, ignoreExtraArgs } = behavior | |
| const isMatchingArgumentCount = ignoreExtraArgs | |
| ? expectedArguments.length <= actualArguments.length | |
| : expectedArguments.length === actualArguments.length | |
| if (!isMatchingArgumentCount) { | |
| return false | |
| } | |
| return expectedArguments.every((expectedArgument, index) => { | |
| return equals(actualArguments[index], expectedArgument) | |
| }) |
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.
The expectedArguments length is defined by the stub the user created with .calledWith(). This rehearsal is expected to be "correct", and so actualArguments length should exactly match in order for the stubbing to be satisfied, unless ignoreExtraArgs is true.
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.
Given the following specification of ignoreExtraArgs...
Check all arguments in
calledWithto find a match, but past the arguments specified incalledWith, ignore any extra arguments in the call itself when determining the match
...withignoreExtraArgs set, what do you expect the following stubbing to do?
const spy = when(vi.fn(), { ignoreExtraArgs: true })
.calledWith(undefined)
.thenReturn('called')
const result = spy() // should this be `called` or `undefined`?My gut says when(vi.fn(), { ignoreExtraArgs: true }).calledWith(undefined) should not match spy(). However, with a !== check, this does match.
Passing undefined explicitly is extremely similar to, but not technically exactly the same as, omitting an argument, so I think if the user says "I expect an explicit undefined" it should be respected, even if ignoreExtraArgs is set
|
I created a new interface |
c130e19 to
a5ff0b7
Compare
|
Ha, it is a bit more difficult than I originally thought 😅 |
|
Okay, everything should be working now. Not sure why the build failed though, looking at the logs all the tasks worked, and it works for me locally 🤷 |
|
Thanks for the contribution! I'll get this released shortly, once I fix some extra typing stuff I just noticed unrelated to this PR |
Borrowed from
testdouble.jsoption of the same name