chore: fail test suite if node encounters unhandled promise rejection #5061
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
karanbirsingh
changed the title
…ee if it makes a difference in console output
…est to see if it makes a difference in console output" This reverts commit c24cf0f.
4 tasks
…nimal fix for now to see if other tests are affected by new flag)
…lace the static config list with something else; this was difficult to grok
dbjorge
approved these changes
Jan 14, 2022
dbjorge
added a commit
that referenced
this pull request
Jan 14, 2022
#5066) #### Details This PR adds a new setup step to each of our test suites which forces Jest to flush all `Promise`s in a globally applied `afterEach` fixture. This forces Jest to flush any pending resolved Promises that a test might have forgotten to handle *before* Jest considers that test to be completed. That way, if a test leaks a Promise, the resulting error message gets logged as a failure in the responsible test and includes a nice, Jest-formatted stack, instead of test "passing" and the error as a Node UnhandledProjmiseRejectionWarning with no stack or test context. This is intended to work in tandem with #5061, allowing it to go in as a last-resort while mitigating the poor logging that we get if it's actually triggered (by avoiding having it need to trigger in the common case) ##### Motivation Avoids silent test failures (eg, the one that currently exists in `injector-controller.test.ts`) ##### Context jestjs/jest#10784 (comment) #### Pull request checklist <!-- If a checklist item is not applicable to this change, write "n/a" in the checkbox --> - [n/a] Addresses an existing issue: #0000 - [x] Ran `yarn fastpass` - [x] Added/updated relevant unit test(s) (and ran `yarn test`) - [x] Verified code coverage for the changes made. Check coverage report at: `<rootDir>/test-results/unit/coverage` - [x] PR title *AND* final merge commit title both start with a semantic tag (`fix:`, `chore:`, `feat(feature-name):`, `refactor:`). See `CONTRIBUTING.md`. - [n/a] (UI changes only) Added screenshots/GIFs to description above - [n/a] (UI changes only) Verified usability with NVDA/JAWS
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
We have a test that is silently failing because of an unhandled promise rejection. Here are snippets from recent logs. injector-controller is failing (missing an extra InjectionCompleted setup):
This PR tries the suggested CLI flag so that our test suite will fail when this happens. Once we have a commit whose associated build fails, I will follow up with a fix for the unit test(s) in this same PR so we can stay green.
The CLI flag appropriately fails the test suite, but I had trouble associating the error with the right test. Debugging updates:
--unhandled-rejections=strictfails the test suite, but it's hard to associate the error with the right test. The process completes before listing the name of the failing test. Locally on Windows + node v14.18.2 I see explicit 'FAIL' for the appropriate test; even with 'runInBand' , the process exists after listing the name of the test that failed.await new Promise(jest.nextTick)after each test produces the same log output as abovecatchblock. That PR will give us better logging if this flag is triggered.Motivation
Unhandled promise rejections should be treated as test failures.
Context
At first I am updating the unit tests with minimal refactoring to make sure we can get green. It would be good to simplify the tests that were affected by this; they were difficult to debug.
Pull request checklist
yarn fastpassyarn test)<rootDir>/test-results/unit/coveragefix:,chore:,feat(feature-name):,refactor:). SeeCONTRIBUTING.md.