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

failing toMatch(Inline)Snapshot does not abort test #10888

Closed
eps1lon opened this issue Nov 29, 2020 · 7 comments · Fixed by #10995
Closed

failing toMatch(Inline)Snapshot does not abort test #10888

eps1lon opened this issue Nov 29, 2020 · 7 comments · Fixed by #10995

Comments

@eps1lon
Copy link
Contributor

eps1lon commented Nov 29, 2020

🐛 Bug Report

A failing toMatch(Inline)Snapshot will not stop the test.

To Reproduce

Steps to reproduce the behavior:

  • Put any statement behind a failing expect(...).toMatchSnapshot()

Expected behavior

Test aborts on the failing line just like other matchers (e.g. toEqual) behave.

Link to repl or repo (highly encouraged)

https://github.com/eps1lon/jest-repro-inline-snapshot-bail

envinfo

  System:
    OS: Linux 5.4 Ubuntu 20.04.1 LTS (Focal Fossa)
    CPU: (6) x64 Intel(R) Core(TM) i5-9400 CPU @ 2.90GHz
  Binaries:
    Node: 14.14.0 - ~/.nvm/versions/node/v14.14.0/bin/node
    Yarn: 1.22.5 - /usr/bin/yarn
    npm: 6.14.8 - ~/.nvm/versions/node/v14.14.0/bin/npm
  npmPackages:
    jest: ^26.6.3 => 26.6.3
@eps1lon
Copy link
Contributor Author

eps1lon commented Dec 2, 2020

Looks like not throwing on mismatch is in fact intended. Though I don't agree with the behavior since it's quite surprising as a long-time jest user.

I'm working around it (where the current semantics of toMatchInlineSnapshot don't make sense) with

function toMatchSpeechInlineSnapshot(recordedSpeech, expectedSpeechSnapshot) {
  // Abort test on first mismatch.
	// Subsequent actions will be based on an incorrect state otherwise and almost always fail as well.
	this.dontThrow = () => {};
}

@SimenB
Copy link
Member

SimenB commented Dec 2, 2020

Comment seems lost at some point, but it's from https://github.com/facebook/jest/pull/1721/files#diff-a9ba5711853a90677041b9a8ec8e241441b46c9f778da77161d3f94e749e4f7eR63-R67. Not really sure why... Maybe so the count of "failing" and "updated" afterwards match up?

@cpojer thoughts on this one?

@eps1lon
Copy link
Contributor Author

eps1lon commented Dec 2, 2020

TL;DR: toMatchSnapshot currently behaves like it's only used in "arrange-act-assert" while any other matcher assumes "arrange-act-assert(-act-assert)+".

Generally, I wouldn't mind being able to tell jest "run the test, and then show me all the failed assertions". This makes sense if you have a single action and multiple assertions that don't necessarily depend on each other.

In

const container = render(<App />);
const button = container.querySelector('button')

expect(button).toHaveFocus();
expect(button).toHaveText('Click me');

it wouldn't hurt to display all failed expectations instead of only displaying the very first.

However, this breaks down once you have tests with multiple actions that are asserted on e.g.

const container = render(<App />);
const button = container.querySelector('button')

expect(button).toHaveFocus();
expect(button).toHaveText('Click me');

button.click()

expect(container.querySelector('dialog')).not.toBe(null);

where you don't need to check if the dialog is opened when we already couldn't find the <button>.

In other words, when strictly following the arrange-act-assert pattern matchers don't need to throw but should just report their failure state at the end. But if you do "arrange-act-assert(-act-assert)+" you do want to stop at the first failed assert-block.

@cpojer
Copy link
Member

cpojer commented Dec 3, 2020

One of the main reasons for doing this in the past was so that we keep Jest running (until something else actually throws) so that snapshots are not reported as obsolete – or removed when u is used. More often than not, this behavior ensures that snapshots only receive minimal changes when iterating on code. I don't think we should change it. To verify what I'm saying, I recommend changing it to throw and playing with the change for a bit. This was mostly a UX thing at the time when we did this.

@eps1lon
Copy link
Contributor Author

eps1lon commented Dec 3, 2020

Yeah, I wouldn't feel comfortable changing it. For my specific use case I need a custom inline snapshot matcher anyway where throwing semantics make more sense.

Just want to clarify whether this.dontThrow = () => {} is considered public API or if you would prefer a better API for throwing immediately?

@eps1lon
Copy link
Contributor Author

eps1lon commented Jan 2, 2021

Added documentation and a test for dontThrow in #10995. I don't need the default behavior changed if there's a documented and tested API for the changed behavior.

@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 Jun 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants