timkindberg / jest-when

Jest support for mock argument-matched return values.
MIT License
734 stars 38 forks source link

Added a verifyExpectedMocksCalled which only asserts on expectCalledWith #64

Open kibiz0r opened 3 years ago

kibiz0r commented 3 years ago

...and also added a flag when.expectCallShouldAssert to opt into disabling the existing behavior of expectCalledWith.

Not sure this is 100% in line with how you'd want to go about this, but if I'm gonna request a feature I can at least take a crack at it, right? :wink:

timkindberg commented 3 years ago

I appreciate the effort on this. :)

I'm thinking it's a bit awkward to set a global when.expectCallShouldAssert in order to change the behavior of the library in this fashion. I'm not claiming that the existing way is perfect, I'm worried this takes us down the path of "pleasing everybody".

I'm thinking the better way to do this is just make multiple individual assertions in your code with expect(fn).toBeCalled(). Would that be so bad? How many calls do you need to assert? I think your code would be more readable this way as well, instead of a dev coming later and needing to scan your code for expectCalledWith they could see clearly at the bottom which ones you expect all clumped together.

kibiz0r commented 3 years ago

I'm thinking it's a bit awkward to set a global when.expectCallShouldAssert in order to change the behavior of the library in this fashion. I'm not claiming that the existing way is perfect, I'm worried this takes us down the path of "pleasing everybody".

Agreed, and agreed. I don't like changing behavior with flags, nor do I like having a vastly different contract for two different calls of the same function. It should probably be a separate thing, but I just wanted to get the ball rolling.

I just wanted to show some code to demonstrate:

  1. ...some behavior that would be nice to have. Whether it supplants/augments existing behavior or becomes a new mechanism.... that's another question from showing how it would work.
  2. ...that I think the behavior should be named something like expectCalledWith.... except that I couldn't think of a name that conveyed that intent and relationship to calledWith without also sounding like a synonym of expectCalledWith, so I just gave up and threw that flag in there to own up to the fact that I really wish I could reuse that real estate, lol.

I also agree that I could just add a expect(fn).toBeCalledWith() line everywhere that I use when(fn).calledWith()... but if I were more okay with writing boilerplate, I wouldn't have found this project. :wink:

I don't think it's necessarily more readable to have expectations at the bottom.

I realize the common wisdom is that tests go:

...and that expectations are just another kind of assertion.

But I think treating them the same way puts you in a weird spot.

It tends to end up like:

I don't like the extra cognitive load of having to deal with a stripped-down version of the expectation as well as the full one. Interaction-based testing already tends toward duplicating the assumptions of the implementation code in the test code. I don't want to think about my implementation when I'm writing a test. I just wanna say what I really mean to say. The more I worry about keeping the call stack happy, the further I stray from TDD nirvana.

I'd rather lean into it and just say: Interaction-based tests are fundamentally different from state/value-based testing. You should separate the two anyway, and your tests should SHOUT that they are two very different things.

I phrase it as:

...and apologize for no transgression. 😆

Not everyone agrees with that, which is fine. But I think there's room for both approaches. Probably. I think. ¯\(ツ)

timkindberg commented 3 years ago

Thanks for the write up and sharing your thoughts :) Let me see... 🤔


but if I were more okay with writing boilerplate, I wouldn't have found this project. 😉

Just a quick aside, I'm not sure I'd describe this project as solely a boilerplate eliminator, it's more of a "let's add argument matching in an elegant way that feels like canonical Jest".


I'm not sure what you mean by Expectation Lite? Do you feel that the setting up of the when matchers is akin to setting up expectations? I could see this... you are essentially saying I'm expecting this function to be called with these arguments, and when it is, I want to return this value.

Why is the regular expectCalledWith not working for you? Or just the assertAllWhenMocksCalled utility?

kibiz0r commented 3 years ago

I'm not sure what you mean by Expectation Lite? Do you feel that the setting up of the when matchers is akin to setting up expectations? I could see this... you are essentially saying I'm expecting this function to be called with these arguments, and when it is, I want to return this value.

Bingo. You have to go 90% of the way there to establish a return value, matching the signature of the call you expect, so then if you want to assert that it was called it's mostly copy-paste and changing one word.

Just a quick aside, I'm not sure I'd describe this project as solely a boilerplate eliminator, it's more of a "let's add argument matching in an elegant way that feels like canonical Jest".

Fair enough. :)

Why is the regular expectCalledWith not working for you? Or just the assertAllWhenMocksCalled utility?

I'd like to be able to establish a pattern in my codebase where it's very clear that: This is a class I'm going to use interaction-based testing on, so I'll set up some baseline canned responses in the beforeEach which are not verified, and when you see additional mocking calls in an individual test case then 90% of the time those are verifiable expectations. And because the whole strategy for this class is to assert on expectations, I'll throw verifyAllWhenMocksCalled or similar into the afterEach.

I haven't tried this, but I gathered from https://github.com/timkindberg/jest-when#assert-the-args that expectCalledWith asserts more aggressively than doing a verify afterwards, because it expects with all matchers every time it is called instead of looking for one matching call after all is said and done. I'm not sure if my understanding is right about that.

As for verifyAllWhenMocksCalled, I can see that that's a little too broad in scope. If I'm testing a service class that has 3 collaborators, I'll want to provide some canned responses for each collaborator in the beforeEach, but I don't necessarily expect all collaborators to be involved in every test case, so I can't just stick verifyAllWhenMocksCalled in the afterEach and call it a day.

I'm okay with backing off of this, btw. I just thought there was an appetite for it based on discussion in other issues/PRs.

timkindberg commented 3 years ago

Ok I think the request is clear to me now... particularly the need to set up some non-asserted calls in a beforeEach that may or may not be called in every test... but then some asserted calls in particular tests. Thanks for explaining a bit more.

I'm still a bit curious why expectCalledWith as-is will not work for you. The behavior you described is 100% accurate. Does it not work for you that it asserts when it's called rather than looking for one call at the end of a test?

kibiz0r commented 3 years ago

Yeah. Asserting early means that you can't describe something like:

const fn = jest.fn();

// Can happen in any order, but all must happen by end of test
fn.expectCalledWith(1);
fn.expectCalledWith(2);
fn.expectCalledWith(3);
timkindberg commented 3 years ago

Hmm ok I think maybe if we can have an elegant name for it ... and I wonder should we just auto revert it in our own afterEach so the devs don't have to, kind of like how jest.setTimeout only works on the current test or block.

And ... should we also call the verifyExpectedMocksCalled for the dev as well? I mean they've opted into the expectCalledWith call... seems like it should do something for sure.

it('tests a thing', () => {
  when.lazyExpectCalls()

  // after test
  // automatically verify expected mocks called
  // automatically reverts to eager expect calls at the end
})

Thoughts?

timkindberg commented 3 years ago

I think we could even make lazy expect calls the default in the next major version... and then have a when.eagerExpectCalls() method that enables immediate assertions.