-
Notifications
You must be signed in to change notification settings - Fork 4
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 function that verifies that all when mocks are called #10
Conversation
Inspired by verifyAllWhenMocksCalled of jest-when.
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.
Interesting! This is one part of jest-when
that I disagree with, and I don't personally think it should be part of a stubbing library. I'm very heavily influence by testdouble.js in this regard, which strongly believes that you shouldn't both create a stub with when
and make assertions about how it is called. (They believe it so strongly that they'll throw an error if you attempt to do so).
I think they make a really compelling case, and I've found it to hold in my own work. If one feels like they need to verify the calls made to a stub, then one or more of the following is probably true:
- The verify isn't needed, because the stub itself already sufficiently constrains the interactions of the subject-under-test via input and output data
- The SUT has a design issue (e.g. uncontrolled side-effects) that adding an assertion simply papers over, rather than encouraging you to fix the SUT
All this is born out of two core beliefs about mocking:
- Mocks are tools to design interfaces and structure more than they are for verifying functionality
- Stubs are a looser form of coupling than assertions, and the less coupling you have between your code and your mock-based tests, the happier you will be in the long run
@BeniRupp I'm curious to hear what you think about all this! I'd love to see your use-case(s) for such a method
Thanks @mcous for your fast response and the interesting feedback! Those are the use cases, why we do call
I agree with you, that a wrong implementation of the application should end in a red test because of a unfulfilled expectation. But it is very helpful to get a pointer, to why the output differs from the expected result. So I think that function will provide a further possibility to verify that the application does work as expected and improves the development workflow. |
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 for the additional context @BeniRupp! I think for philosophical (my initial comment yesterday) and technical (my comments on the code) reasons, this feature doesn't fit well into this library.
I think, though, that the debugging needs you pointed out are valid, and I'd like this library to better support the ability to add these sorts of features on externally. Is there an API (or are there APIs) that you could imagine for vitest-when
that:
- Would enable you to build such a verification feature externally, and
- Do not add global state to the library itself?
import type { AnyFunction } from './types.ts' | ||
|
||
export type { WhenOptions } from './behaviors.ts' | ||
export * from './errors.ts' | ||
|
||
export const behaviorStackRegistry = new Set<BehaviorStack<AnyFunction>>() |
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 introduces global state into the library, which requires cleanup. I don't think I want that added complexity in this library. I think it would be confusing to require both vi.resetAllMocks
and some other resetAllWhenMocks
const uncalledMocks = [...behaviorStackRegistry].flatMap((behaviorStack) => { | ||
return behaviorStack.get().filter(behavior => behavior.times && behavior.times > 0) | ||
}) | ||
assert.equal(uncalledMocks.length,0, `Failed verifyAllWhenMocksCalled: ${uncalledMocks.length} mock(s) not called:`) |
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 don't think this assertion message is very helpful, and I think that shows a technical limitation to trying to include this kind of functionality inside of this library. I also think it miscommunicates to the user a little; there is a 1 to many relationship between vi.fn()
mocks and entries in the behavior stack
@BeniRupp did you have any additional thoughts after my comments/review above? If not, I think I’m going to close this PR due to misalignment with project goals. Regardless, I’m very happy to continue this conversation about more debugging capabilities! |
Hi @mcous, sorry, I did not find the time to work on that issue. So I have no additional ideas at the moment and it's fine for me to close this PR. I let you know if I can figure out another debugging idea. 👍 |
Inspired by verifyAllWhenMocksCalled of jest-when.