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 expect.toPass to non-literal value assertions #5819

Closed
kentcdodds opened this issue Mar 17, 2018 · 14 comments
Closed

Add expect.toPass to non-literal value assertions #5819

kentcdodds opened this issue Mar 17, 2018 · 14 comments

Comments

@kentcdodds
Copy link
Contributor

Do you want to request a feature or report a bug?

Feature

What is the current behavior?

Right now I have to write a test like this:

test('some test', () => {
  const arg = {a: 'b', c: Math.random()}
  const subject = jest.fn()
  subject(arg)
  expect(subject).toHaveBeenCalledWith({
    a: 'b',
    c: expect.any(Number),
  })
  expect(subject.mock.calls[0][0].c).toBeLessThanOrEqual(1)
})

I'd prefer to write this test like this:

test('some test', () => {
  const arg = {a: 'b', c: Math.random()}
  const subject = jest.fn()
  subject(arg)
  expect(subject).toHaveBeenCalledWith({
    a: 'b',
    c: expect.toPass(val => expect(val).toBeLessThanOrEqual(1))
  })
})

I'm fine if we call it something else or if the API is different, but this would be really helpful :)

Note, with my proposed API you could write multiple assertions:

test('some test', () => {
  const arg = {a: 'b', c: Math.random()}
  const subject = jest.fn()
  subject(arg)
  expect(subject).toHaveBeenCalledWith({
    a: 'b',
    c: expect.toPass(val => {
      expect(val).toBeGreaterThanOrEqual(0)
      expect(val).toBeLessThanOrEqual(1)
    })
  })
})

Thoughts?

@rickhanlonii
Copy link
Member

Solid feature request 👍

There are two ways to implement this with custom asymmetric matchers

The first is with the proposed API:

const toPass = fn => ({
  asymmetricMatch: actual => {
    return fn(actual) || true;
  },
});

it("some test", () => {
  const arg = {a: 'b', c: Math.random()}
  const subject = jest.fn()
  subject(arg);
  expect(subject).toHaveBeenCalledWith({
    a: 'b',
    c: toPass(val => {
      expect(val).toBeGreaterThanOrEqual(0)
      expect(val).toBeLessThanOrEqual(1)
    }),
  })
});

The error output for this approach is:

The second approach is to create an asymmetric matcher specific to what you're testing:

const between = (start, end) => ({
  $$typeof: Symbol.for('jest.asymmetricMatcher'),
  asymmetricMatch: actual => {
    return start < actual && actual < end
  },
  toAsymmetricMatcher: () => {
    return `<Between ${start} and ${end}>`;
  }
});

it("some test", () => {
  const arg = {a: 'b', c: Math.random() + 1}
  const subject = jest.fn()
  subject(arg);
  expect(subject).toHaveBeenCalledWith({
    a: 'b',
    c: between(0, 1),
  })
});

To me, this has a more expected output (that the arguments the mock was called with were not what was expected). We're working to make the diff more informative so assume here that the diff is as direct as the above strategy:

@kentcdodds what are your thoughts on these two strategies? Also, is there a convincing reason for why this should be baked directly into Jest instead of userland or jest-extended?

@kentcdodds
Copy link
Contributor Author

My example is just an example. I like both solutions, but for the built-in solution I think your first example is better. It's the most generic solution as it allows me to throw any error whether it's an assertion or otherwise.

I'd be fine with both :)

As for whether this should be baked in, I would personally use this all the time and I doubt I'm alone. One of the things I love about jest is that batteries are included for most common use cases and I think this is a pretty common use case so it seems like this is a battery to include.

@rickhanlonii
Copy link
Member

Ah, got it ☺️

Will wait for others to give feedback, but I also want to mention that once master lands in a release you will be able to do this with custom asymmetric matchers:

expect.extend({
  toPass(received, fn) {
    return fn(received);
  }
})

it("some test", () => {
  const arg = {a: 'b', c: Math.random() * 2}
  const subject = jest.fn()
  subject(arg);
  expect(subject).toHaveBeenCalledWith({
    a: 'b',
    c: expect.toPass(val => {
      expect(val).toBeGreaterThanOrEqual(0)
      expect(val).toBeLessThanOrEqual(1)
    }),
  })
});

@kentcdodds
Copy link
Contributor Author

That's super! Thanks for showing that. It's way easier! I'd still say that this is something that would be beneficial for everyone to have built-in, and it looks like it'd be pretty easy to do that 👍

@SimenB
Copy link
Member

SimenB commented Mar 18, 2018

Doesn't #5503 cover this use case? It doesn't cover multiple assertions, but is that common enough?

@SimenB
Copy link
Member

SimenB commented Mar 18, 2018

#5788 would be better, I think (if we were to do either)

@aymericbouzy
Copy link
Contributor

aymericbouzy commented Mar 19, 2018

#5503 almost covers the first use case with the following syntax:

test('some test', () => {
  const arg = {a: 'b', c: Math.random()}
  const subject = jest.fn()
  subject(arg)
  expect(subject).toHaveBeenCalledWith({
    a: 'b',
    c: expect.toBeLessThanOrEqual(1),
  })
})

It would be needed to extend the new power of expect.extend to predefined matchers.

Still with #5503, we could do the second use case in the following way:

expect.extend({
	toPass(received, doesPass) {
		try {
			doesPass(received)
			return { pass: true }
		} catch (error) {
			return { pass: false }
		}
	},
})

it("some test", () => {
  const arg = {a: 'b', c: Math.random()}
  const subject = jest.fn()
  subject(arg);
  expect(subject).toHaveBeenCalledWith({
    a: 'b',
    c: expect.toPass(val => {
      expect(val).toBeGreaterThanOrEqual(0)
      expect(val).toBeLessThanOrEqual(1)
    }),
  })
});

but my feeling is that it's a bit of a hack. It should work though. The error message will be very cryptic, which is not a good thing in my opinion.

With #5788, it's even more straightforward:

it("some test", () => {
  const arg = {a: 'b', c: Math.random()}
  const subject = jest.fn()
  subject(arg);
  expect(subject).toHaveBeenCalledWith({
    a: 'b',
    c: expect.toBeGreaterThanOrEqual(0).toBeLessThanOrEqual(1)
    }),
  })
});

assuming once more that we extend predefined matchers to be both symmetric and asymmetric. The error message would be more explicit in that case (we could print the specific matcher that fails in the chain), but still less explicit than the symmetric matcher of course.

@kentcdodds
Copy link
Contributor Author

None of the proposals seem nearly as straightforward to me as my original proposal/desired API. I understand the desire to push back on expanding the surface area of the built-in API, but I really feel like this would be an enhancement that would reduce the number of questions about how to accomplish this kind of thing. The suggested alternatives are definitely not nearly as obvious.

@mattphillips
Copy link
Contributor

This API currently exists in jest-extended see toSatisfy.

it("some test", () => {
  const arg = {a: 'b', c: Math.random()}
  const subject = jest.fn()
  subject(arg)
  expect(subject).toHaveBeenCalledWith({
    a: 'b',
    c: expect.any(Number)
  });

  expect(subject.mock.calls[0][0].c).toSatisfy(n => {
    expect(n).toBeGreaterThanOrEqual(0);
    expect(n).toBeLessThanOrEqual(1);
  });
});

Once custom asymmetric matchers is released in Jest then jest-extended can also offer all APIs asymmetrically and the above will become:

it("some test", () => {
  const arg = {a: 'b', c: Math.random()}
  const subject = jest.fn()
  subject(arg)
  expect(subject).toHaveBeenCalledWith({
    a: 'b',
    c: expect.toSatisfy(n => {
      expect(n).toBeGreaterThanOrEqual(0);
      expect(n).toBeLessThanOrEqual(1);
    });
  });
});

@kentcdodds
Copy link
Contributor Author

That's awesome!

Could I petition toSatisfy be added to the core? Like I said, most people don't include (or even know about) jest-extend and end up writing lengthy assertions like the one I have above. If this were included in the core that problem would go away. I realize that jest can't add every handy assertion possible, but this one seems pretty significantly useful to me.

@mattphillips
Copy link
Contributor

I wonder if it would be a good idea to include jest-extended as part of the core Jest installation and allow for user to override the version of jest-extended that way the two projects can move at different rates with jest-extended's additional APIs also available by default.

I appreciate this may be not be relevant to this issue, I'm curious to gauge interest and can open a separate issue if needs be.

@rickhanlonii
Copy link
Member

TIL @mattphillips, I missed that one 👍

We're not going to add this to Jest core right now. We encourage people to use jest-extended, custom asymmetric matchers, and custom matchers, so avoiding them is not a compelling argument to add it to core 🤙

@kentcdodds
Copy link
Contributor Author

Ok <3

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants