Skip to content
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

Closed

Conversation

BeniRupp
Copy link

@BeniRupp BeniRupp commented Feb 6, 2024

Inspired by verifyAllWhenMocksCalled of jest-when.

Inspired by verifyAllWhenMocksCalled of jest-when.
Copy link
Owner

@mcous mcous left a 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

@BeniRupp
Copy link
Author

BeniRupp commented Feb 9, 2024

Thanks @mcous for your fast response and the interesting feedback!

Those are the use cases, why we do call verifyAllWhenMocksCalled() in a global afterEach hook in our project:

  • We want to know if our application, for whatever reason, didn't do the calls that we mock (and therefore expect) in our tests. During refactoring for example.
  • We want to have a minimal test setup. The verification helps us, especially when copy-pasting a complex test setup, to get rid of unnecessary mocks.

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.

Copy link
Owner

@mcous mcous left a 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:

  1. Would enable you to build such a verification feature externally, and
  2. 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>>()
Copy link
Owner

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:`)
Copy link
Owner

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

@mcous
Copy link
Owner

mcous commented Mar 10, 2024

@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!

@BeniRupp
Copy link
Author

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. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants