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

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

Open
narthur opened this issue May 2, 2023 · 12 comments
Open

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

narthur opened this issue May 2, 2023 · 12 comments

Comments

@narthur
Copy link

narthur commented May 2, 2023

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
Copy link
Author

narthur commented May 2, 2023

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
Copy link
Owner

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

@narthur
Copy link
Author

narthur commented May 2, 2023

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
Copy link
Owner

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
Copy link

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

@timkindberg
Copy link
Owner

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

@narthur
Copy link
Author

narthur commented May 3, 2023

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
Copy link

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

sinonjs/sinon#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
Copy link
Author

narthur commented May 3, 2023

@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
Copy link
Owner

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
Copy link

joebowbeer commented May 4, 2023

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
Copy link

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.

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

No branches or pull requests

4 participants