Integration test fixes in light of new Promise warnings #7
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
There were test failures when I'd left `HUBOT_LOG_LEVEL=debug` set in the environment, which added `DEBUG` messages to the logging output.
As discovered in 18F#51, the `UnhandledPromiseRejectionWarning` and `PromiseRejectionHandledWarning` warnings were apparently added in v6.6.0 (https://nodejs.org/en/blog/release/v6.6.0/); specifically it was added by nodejs/node#8223. See also: nodejs/node#6439 nodejs/node#8217 https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_unhandledrejection https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_rejectionhandled https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_warning Test failures from `test/integration-test.js` after upgrading to Node v6.9.1 showed extra output such as: ``` (node:39696) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): null (node:39696) PromiseRejectionHandledWarning: Promise rejection was handled asynchronously (rejection id: 2) ``` This was happening because `scripts/slack-github-issues.js` created a Hubot listener that returned a `Promise` so that the integration test could use `.should.be.rejectedWith` assertions. Rather than jump through hoops to quash the warnings, this change now causes the listener's `catch` handler to return the result of the rejected `Promise`, converting it to a fulfilled `Promise` in the process. Since Hubot never used the result anyway, the only effect it has in production is to eliminate the warning messages. The tests now just check that the `Promise` returned by the listener callback is fulfilled with the expected error result, with practically no loss in clarity.
mbland
added a commit
to mbland/unit-testing-node
that referenced
this pull request
Apr 28, 2017
Backported from mbland/hubot-slack-github-issues#7. As discovered in 18F/hubot-slack-github-issues#51, the `UnhandledPromiseRejectionWarning` and `PromiseRejectionHandledWarning` warnings were apparently added in v6.6.0 (https://nodejs.org/en/blog/release/v6.6.0/); specifically it was added by nodejs/node#8223. See also: nodejs/node#6439 nodejs/node#8217 https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_unhandledrejection https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_rejectionhandled https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_warning A test failure from `solutions/06-integration/test/integration-test.js` after upgrading to Node v6.9.1 showed output such as: ``` - "(node:87412) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 14): Error: failed to create a GitHub issue in 18F/handbook: received 500 response from GitHub API: {\"message\":\"test failure\"}\n" ``` This was happening because `scripts/slack-github-issues.js` ignored the return value from `Middleware.execute()`, whether it was undefined or a `Promise`. For consistency's sake (and to provide a clearer upgrade path to the current state of mbland/slack-github-issues), `Middleware.execute()` always returns a `Promise`, and `scripts/slack-github-issues.js` handles and ignores any rejected Promises. This fixed the `integration-test.js` error, but also required minor updates to `solutions/{05-middleware,complete}/test/middleware-test.js`. The next commit will update the tutorial narrative to account for this change.
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.
As discovered in 18F#51, the
UnhandledPromiseRejectionWarningandPromiseRejectionHandledWarningwarnings were apparently added in v6.6.0 (https://nodejs.org/en/blog/release/v6.6.0/); specifically it was added by nodejs/node#8223. See also:nodejs/node#6439
nodejs/node#8217
https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_unhandledrejection
https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_rejectionhandled
https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_warning
Test failures from
test/integration-test.jsafter upgrading to Node v6.9.1 showed extra output such as:This was happening because
scripts/slack-github-issues.jscreated a Hubot listener that returned aPromiseso that the integration test could use.should.be.rejectedWithassertions. Rather than jump through hoops to quash the warnings, this change now causes the listener'scatchhandler to return the result of the rejectedPromise, converting it to a fulfilledPromisein the process.Since Hubot never used the result anyway, the only effect it has in production is to eliminate the warning messages. The tests now just check that the
Promisereturned by the listener callback is fulfilled with the expected error result, with practically no loss in clarity.Also, there were test failures when I'd left
HUBOT_LOG_LEVEL=debugset in the environment, which addedDEBUGmessages to the logging output. So there's also a commit to clearHUBOT_LOG_LEVELin the test set up.