timkindberg / jest-when

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

Deprioritize `expect.anything()` when applying `calledWith` matchers #104

Open narthur opened 1 year ago

narthur commented 1 year ago

I have a function where the first argument is an identifier and the second argument is an options object. Usually I don't care about the options object, so I frequently load my default data using expect.anything() for the second argument.

However for one test I need to load my data based on whether or not the options object contains some specific values. So I would like to be able to use expect.objectContaining to match against the options object without making the test too brittle by matching against the whole thing.

After doing some testing, it appears that jest-when doesn't currently support this, as expect.anything() is matched even though expect.objectContaining() would also match the call. I would prefer that jest-when would deprioritize expect.anything() over all other asymmetric matchers.

I'm running jest-when 3.5.2 and jest 27.5.1.

Here are the tests I wrote to nail down the issue:

it('supports expect.objectContaining()', async () => {
    const fn = jest.fn();

    when(fn).mockReturnValue(false);
    when(fn)
        .calledWith(
            'id',
            expect.objectContaining({
                k: 'v',
            })
        )
        .mockReturnValue(true);

    // this test passes
    expect(
        fn('id', {
            k: 'v',
        })
    ).toBeTruthy();
});

it('deprioritizes expect.anything() against literal values', async () => {
    const fn = jest.fn();

    when(fn).calledWith('id', expect.anything()).mockReturnValue(false);
    when(fn)
        .calledWith('id', {
            k: 'v',
        })
        .mockReturnValue(true);

    // this test passes
    expect(
        fn('id', {
            k: 'v',
        })
    ).toBeTruthy();
});

it('deprioritizes expect.anything() against other asymmetric matchers', async () => {
    const fn = jest.fn();

    when(fn).calledWith('id', expect.anything()).mockReturnValue(false);
    when(fn)
        .calledWith(
            'id',
            expect.objectContaining({
                k: 'v',
            })
        )
        .mockReturnValue(true);

    // this test fails
    expect(
        fn('id', {
            k: 'v',
        })
    ).toBeTruthy();
});
narthur commented 1 year ago

I'm guessing this would be really hard to do, but I think what my ideal would be is for matchers to have priority based on their specificity. E.g.:

it('accounts for assymetric matcher specificity', async () => {
    const fn = jest.fn();

    when(fn)
        .calledWith(
            expect.objectContaining({
                id: 'id',
            })
        )
        .mockReturnValue(false);
    when(fn)
        .calledWith(
            expect.objectContaining({
                id: 'id',
                k: 'v',
            })
        )
        .mockReturnValue(true);

    // this test fails
    expect(
        fn({
            id: 'id',
            k: 'v',
        })
    ).toBeTruthy();
});
timkindberg commented 1 year ago

what if they are defined in the opposite order do the tests pass then?

narthur commented 1 year ago

Yes! Though manually flipping the order of the calledWith calls like that doesn't work for my use case since I'm defining the default return value in beforeEach. Are you thinking to flip the order in which matchers are checked internally, so that more-recently-added matchers have priority? 🤔

timkindberg commented 1 year ago

Hmm no I wasn't but maybe that's possible. I'm not sure it would be wholly impossible to do this sort of prioritizing, however I'm not going to have time ATM. If you'd like to give it a shot feel free!!

joebowbeer commented 1 year ago

This proposal sounds like a breaking change, and I think it might be more confusing than it is intuitive.

timkindberg commented 1 year ago

You're right this would be a breaking change... good callout!

narthur commented 1 year ago

Yes, I agree that everything discussed here would be breaking, and the specificity idea definitely risks making things less understandable than they already are. Though the amount of time I spent figuring out why my matcher wasn't working has me thinking that the status quo isn't very intuitive in some situations, either!

@joebowbeer Would you also feel that reversing the order of matching would also make things less intuitive? I think that I'd be more likely to expect that a newer call would have precedence over an older call, but maybe others would disagree?

joebowbeer commented 1 year ago

@narthur I don't think any other way is better enough to warrant breaking backing compatibility, but I think there could be an interesting debate about this on a green field somewhere in the future.

I think the way jest-when works is intuitive if you think of it as chaining (fluent-style). The handlers are called in the order in which they were chained, with no backtracking.

However, there are aspects of jest-when that complicate this mental model, such as training replacements and default implementations.

In sinon, the last match wins, but this may have been an afterthought:

https://github.com/sinonjs/sinon/pull/1183

In css, it is reported above that the most specific match wins, but I think this would be impractical in Typescript.

Would matching allArgs solve your use case?

https://github.com/timkindberg/jest-when#supports-matching-or-asserting-against-all-of-the-arguments-together-using-whenallargs

narthur commented 1 year ago

@joebowbeer Hmm, I'm not sure. From the documentation I would assume I'd run into the same issue if I had multiple calledWith > allArgs calls--the first match would be taken. So I'm guessing I'd need to encapsulate a single call to calledWith and store the matchers and have the single allArgs matcher reference the stored list of matchers. And at that point why am I using jest-when? I should just write my own custom jest mock and call it a day.

Or it's also quite likely I'm missing the idea, since I haven't used allArgs before.

timkindberg commented 1 year ago

If I were writing things from scratch I think I like the idea of most recent match winning. TBH I never anticipated someone having two calledWiths that could match... kinda obvious now but yeah didn't really think that that would happen. I use fetchMock to mock out api calls from my UI and I have it set up so that newer mock returns on the same api clobber older mocks, so new wins in that case and it's intuitive. Because usually the older one is in a beforeEach as a general mock and the newer mock is in a particular test.

I'm actually wondering how often ppl do this, where they have a mock fn that has multiple potentially matching calledWiths registered 🤔 .

Like it might be a breaking change but it also might not be an issue in 80%+ use cases. It's funny that this is the first we are hearing about it. I wonder how ppl have gotten around this thus far, or maybe they just never ran into this issue?

joebowbeer commented 1 year ago

I think this comes up less often in jest-when because defaultImplementation exists and is order-independent. That handles the general catch-all (e.g., wrongArgs) case, which I would list first in sinon, but can appear anywhere in jest-when.

Then the training replacement feature handles the cases where the last behavior replaces a previous one for a matching set of args.

This only leaves a few use cases where first-match wins.

camsteffen commented 1 month ago

If you need complex matching logic, then you can collapse those cases into one mockImplementation and do it manually. Relying on "specificity" between multiple mocks is too implicit IMO, and doesn't save much code anyways.