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

Allow fail to work in try/catch #664

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

thehale
Copy link

@thehale thehale commented Oct 9, 2023

What

This PR allows calls to expect().fail(message) to fail a test, even when the expect().fail() is executed inside of a try/catch block.

Why

closes #663

This functionality has been desired in Jest since the original fail function was removed from the imported Jasmine scope.

Throwing an error is semantically different from failing a test, and attempting to equate the two makes it much harder to test code which uses try/catch blocks.

Notes

I am unaware of any way for Jest to report an "expected failure" similar to pytests xfail decorator. The closest I could find was prefixing the test as xtest to skip it. Deleting those x prefixes allows one to manually inspect the test failure messages to verify they line up with the expected behavior.

I am happy to edit the PR if there's a better way to record "expected failures".

EDIT: The latest commit on the PR now validates that the tests fail as expected instead of skipping them.

Housekeeping

  • Unit tests
  • Documentation is up to date
  • No additional lint warnings
  • Typescript definitions are added/updated where relevant

@changeset-bot
Copy link

changeset-bot bot commented Oct 9, 2023

⚠️ No Changeset found

Latest commit: 01b1275

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@SimenB
Copy link
Member

SimenB commented Oct 10, 2023

Having skipped tests doesn't make much sense. We probably need some sort of integration test which spawns Jest or something for this case

@thehale
Copy link
Author

thehale commented Oct 16, 2023

I've never done something like that before but am happy to put in the work to make it happen. Some initial ideas...

  1. Invoke jest via the CLI for a sample file and run some assertions on the CLI output.
    • Feels brittle, any CLI change could break the test
  2. Mock out the test results collector and verify that calling fail reports a failed test.
    • Seems too "omniscient" about jest internals.
  3. Something else?

Which approach would you recommend I pursue?

@liamjones
Copy link

I'm also looking for this functionality but I don't think this solution works as-is.

xtest aka test.skip is, as SimenB said, is skipping the test.

I think what you wanted instead was test.failing which is considered good when the test fails and bad when it passes.

If I checkout your PR and change the xtest to test.failing the test, incorrectly, passes.

@thehale
Copy link
Author

thehale commented Dec 22, 2023

@liamjones The docs seem to suggest that test.failing only flips the expectation if the test throws an error.

If failing test will throw any errors then it will pass. If it does not throw it will fail.

https://jestjs.io/docs/api#testfailingname-fn-timeout

Since this PR directly reports a test as failing instead of throwing an exception for Jest to catch, test.failing never catches a JestException, causing it to mistakenly believe the test passed.

Notice that by changing xtest to test, the tests do fail as expected.

@thehale
Copy link
Author

thehale commented Dec 23, 2023

After digging into all this some more, I figured out a way to use a custom jest reporter to detect the expected test failures and flip them to passing.

The custom reporter also marks these "expected failing" tests as actually failing if they unexpectedly pass.

The solution is decently complex, so I'm happy to answer questions about how it works and implement suggestions to make the implementation more approachable.

To verify that the new `fail` matcher works correctly, we need a way to
check if the tests failed as expected or not.

Typically one would use `test.failing` for this purpose, but that only
works if the test threw an exception (e.g. JestException). The `fail`
matcher does not do this so that it can still work inside `catch`
blocks.

Instead, we have to hook into the test reporting and mutate the test
results for our expected test failures prior to usage by other test
reporters.

This commit creates a custom test reporter which detects tests run in
the file `fail.test.js` (yes the name is hard-coded) and flips the
results of tests inside a `.fail` describe block (i.e. our expected
failures).
 - If the tests failed (expected) we flip the result to a pass.
 - If the tests passed (unexpected) we flip the result to a fail.

The custom reporter also handles the logic for updating the counts for
failing test suites so that later reporters reflect the actual test
results correctly.
@thehale
Copy link
Author

thehale commented Jan 10, 2024

@SimenB This PR is ready for another maintainer review.

P.S. I see per your GitHub status that you are currently "busy"/"slow to respond", so no rush!

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

Successfully merging this pull request may close these issues.

fail does not work when testing code using a try/catch
3 participants