[Bug]: test.failing() does not work with failing snapshot

[mrazauskas]

Version

28.1.0

Steps to reproduce

Seems like jest.failing() does not work with failing snapshots. Here is a reproduction: https://github.com/mrazauskas/x-jest-failing

Snapshot is failing inside test.failing(), but the output is: "Failing test passed even though it was supposed to fail." And later: "1 snapshot failed".

@michalwarda Would you like to take a look? (;

Expected behavior

Might be I misunderstood something, but it felt like we should see pass instead of fail.

Actual behavior

Additional context

No response

Environment

System:
    OS: macOS 12.3.1
    CPU: (10) arm64 Apple M1 Pro
  Binaries:
    Node: 18.0.0 - /opt/homebrew/bin/node
    Yarn: 1.22.18 - ~/.yarn/bin/yarn
    npm: 8.6.0 - /opt/homebrew/bin/npm
  npmPackages:
    jest: ^28.1.0 => 28.1.0

[SimenB]

related, what should happen in a failing test with a snapshot if it's updated?

[michalwarda]

welp...

This might be harder than I anticipated :D. After some initial research I think I found that snapshot matchers behave differently to other matchers.

Tests that fail on snapshot matching are not actually failing (they are not sending a test_fn_failure event but a normal test_fn_success).

From what I found the failure logic is checked after the runner logic in reporters. (at least that's what I think is happening).

To fix this we would probably need to add the failing logic also to all reporters.
Second option is that we can try to remove this edge behavior from snapshots tests. Though it seems like a big task and it's potentially breaking.

Though I might be completely wrong here!

[SimenB]

😅

We should start by adding a caveat to the docs at least

[mrazauskas]

Just to add, I bump it this while trying to use failing instead of skip flag here: https://github.com/facebook/jest/blob/8e8ee90d8dd583294fc95955e51f373d4c2fea40/e2e/__tests__/complexItemsInErrors.ts#L64-L66

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[mrazauskas]

Bump. Has to be fixed.

[phmngocnghia]

related, what should happen in a failing test with a snapshot if it's updated?

If the snapshot is updated, then the statement would pass inside of test.failing. Does this mean this test case is failed? For this case, I think we can:

• Add a flag to ignore snapshot in test.failing
• Add documentation about updating snapshot would update the fail snapshot in test.failing
• Make fail snapshots test.fail pass the test

cc: @SimenB I interest in working on this issue

[Raaghu]

+1

[KhaledElmorsy]

Should snapshots in test.failing be open to updates? Intuitively, I'd say no, because expecting a snapshot to fail is essentially an expect().not.toEqual() test. I've managed a preliminary fix but want to confirm the update workflow to see if I need to to account for it.

[mrazauskas]

I think all snapshots inside test.failing should be excluded from updating:

1. Failing snapshot will pass the test, so there is no need to update it.
2. Passing snapshot will simply print a message suggesting to remove the failing modifier, there is no need to touch the snapshot as well.
3. Can be there are both passing and failing snapshots inside a single test.failing. In this case it is hardly possible to detect which are the bad ones, so better to not touch anything.

Are there any more scenarios?

[KhaledElmorsy]

Agreed. Don't think there are more scenarios.

Also, the reason the snapshots fail is because their errors are suppressed when the test's running, then they're processed after the test, in one of the test_done event handlers. Another matcher that's processed with it is expect.assertions(). So the test below fails too even though it should pass.

test.failing('Assertions', () => {
  expect.assertions(0);
  expect(1).toBe(1);
});

I plan to fix both cases.

[mrazauskas]

Hm.. Not sure about this case. For me it looks like someone made a mistake using expect.assertions().

[KhaledElmorsy]

It's not an intuitive usage, but it does technically affect if a test passes or fails. So if someone expects a failing test to pass because of test.failing, then this would be one of those failing conditions. So I guess it's more of technical interaction, not a practical one xD.

If you want, I can just focus on the snapshot bug, the two cases need different types of solutions either way.

[mrazauskas]

If you want, I can just focus on the snapshot bug, the two cases need different types of solutions either way.

Yes, it would be great to make snapshots work with test.failing.

[KhaledElmorsy]

Should failed snapshots stop evaluation? test.failing would pass at the first one, so I'd say it's a waste of resources to evaluate them all, especially since other matchers in the same test could throw.

Docs also say:

If failing test will throw any errors then it will pass. If it does not throw it will fail.I'm already preserving them regardless of if the test throws or not, so that wouldn't be a problem if we throw on the first failed snapshot.

But disclaimers could always be added.

In terms of preserving unchecked tests, which is one of the potential issues, I've already handled that regardless of the decision.

[mrazauskas]

Hm.. I think execution of test.failing should stop after first failing assertion. This sounds right. Is the current logic different? If so and if you see how to fix that, better open a separate PR. Sound right to improvement performance.

[KhaledElmorsy]

I agree. Snapshots right now get their errors suppressed in expect to log all failing snapshots. This is also the root cause of this issue. Because the test.failing logic runs when the tests throw. I'll add a comment in the PR on how I've handled this.

[millsp]

Also coming to report here that test.failing does not work as expected with expect.assertions. In my example, when the test is successful, we expect expect.assertions(2). If things don't go according to plan, the test yields 0 assertions, and expect.assertions(2) will throw an error. There's no way to suppress this with .failing, the error still bubbles up. I also encountered the snapshot variant.

[SimenB]

#14313

[SimenB]

@millsp could you open up a separate issue with that (including a minimal reproduction)? 🙂

[SimenB]

https://github.com/jestjs/jest/releases/tag/v30.0.0-alpha.1

[github-actions]

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.