fix(expect, jest-snapshot): Pass test.failing tests when containing failing snapshot matchers

[KhaledElmorsy]

Summary

Fix an issue where failed snapshot matchers didn't trigger tests called with test.failing to pass.

With this change, Snapshot matchers in test.failing will:

• Throw on failure, causing the test to pass with no further evaluation.
• Not update their snapshots. Regardless of the snapshot's value/existence.

Test plan

E2e tests were added to ensure the correct test outcome and to ensure that snapshots didn't update.

[netlify]

✅ Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	ad2b713
🔍 Latest deploy log	https://app.netlify.com/sites/jestjs/deploys/651b1fe58e46a70008f5aee8
😎 Deploy Preview	https://deploy-preview-14313--jestjs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[mrazauskas]

I see you are still working. Just few quick notes:

• expect is used a lot outside of Jest. Hence shipping jest-circus with it would be bad idea.
• also note that snapshot matchers are implemented via @jest/expect package and expect does and should not know nothing about them
• it would be bad idea to import jest-circus into @jest/expect as well, because jest-circus imports @jest/expect

Rather complicated. Hope this saves your time.

[KhaledElmorsy]

@mrazauskas Thanks Tom. What do you think of using an optional dynamic import in expect that checks if jest-circus is available before getting and passing the runner state as an optional property? Essentially making it a circus only feature (like test.failing).

[mrazauskas]

TypeScript does not allow to have circular references between packages. So importing jest-circus into expect and later importing expect back into jest-circus will not work.

That’s just technical peculiarity, but in general I think this problem should be sorted out inside the runner. It does not feel like matchers should know something about the runner or its state. test.failing is runners responsibility (similar to test.concurrent, test.each, test.only, test.skip or any other modifier).

[KhaledElmorsy]

That makes sense, thank you. Fortunately this could be a blessing in disguise because of some limitations with maintaining the snapshots that fail, while making them throw errors instead of suppressing them. Back to the drawing board!

[KhaledElmorsy]

I've adapted my PR (local) to not use any circus imports by adding adding and setting a property in expect's state (name can be dontSuppress/alwaysThrow/testFailing) that is then used in jest-snapshot to change it into a throwing, pure matcher.

This ends up solving the root cause, allows execution to stop after the first failure, and simplifies post-processing in Circus to just preserving unchecked snapshots. Is this okay? I know we'd prefer to have a pure runner based solution with no state exposure, but would you be okay with reviewing it?

[mrazauskas]

To be honest, I don’t know the logic snapshot matchers logic in detail. Perhaps others will bring in more feedback.

What I know: @jest/expect was introduced (see #12404) to separate expect and snapshot matchers. So it feels strange to see expect being involved in something what is used only by snapshot matchers.

Perhaps it is possible to involve SnapshotState somehow?

[KhaledElmorsy]

I moved the property to @jest/expect. I kept it as a general MatcherState in case of future uses/fixes in other matchers. Is there anything more I can do for this PR?

[KhaledElmorsy]

Hi @SimenB, could you please review this PR for approval?

Also, the failed tests are all Jasmine runs which exit with code 1 because when I skip the tests in the file, it registers obsolete snapshots.

Is there a way to skip the test file completely when Jasmine is the runner?

[KhaledElmorsy]

@SimenB Any updates?

[SimenB]

Looks like jasmine test is failing on CI. Might need to skip them

[KhaledElmorsy]

What would be a good way to do that?

One that comes to mind is to edit jest.config.mjs with:

{
//..
 testPathIgnorePatterns: [
    '/__arbitraries__/',
    '/__benchmarks__/',
    '/__fixtures__/',
    ...(process.env.JEST_JASMINE ? [
      'e2e/.../'
     ] : [])
  ],
//..
}

[KhaledElmorsy]

@SimenB We're all green ✅

[SimenB]

thank you for a fantastic PR and your patience!

[SimenB]

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

[github-actions]

This pull request 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.