Skip to content

Commit

Permalink
scripts: Always convert result to resolved Promise
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mbland committed Dec 1, 2016
1 parent 1bcfc23 commit 4534793
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 12 deletions.
7 changes: 5 additions & 2 deletions scripts/slack-github-issues.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,13 @@ function fileIssue(filer, response) {
return issueUrl;
})
.catch(function(err) {
var result = err;

if (err) {
response.reply(err.message || err);
result = err.message || err;
response.reply(result);
}
return Promise.reject(err);
return result;
});
}

Expand Down
19 changes: 9 additions & 10 deletions test/integration-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ chai.should();
chai.use(chaiAsPromised);

describe('Integration test', function() {
var room, listenerCallbackPromise, logHelper, apiStubServer, config,
var room, listenerResult, logHelper, apiStubServer, config,
apiServerDefaults, reactionAddedMessage, patchReactMethodOntoRoom,
patchListenerCallbackAndImpl, sendReaction, initLogMessages,
wrapInfoMessages, matchingRule = 'reactionName: evergreen_tree, ' +
Expand Down Expand Up @@ -158,7 +158,7 @@ describe('Integration test', function() {
});

listener.callback = function(response) {
listenerCallbackPromise = callback(response);
listenerResult = callback(response);
};
};

Expand All @@ -179,7 +179,7 @@ describe('Integration test', function() {
sendReaction = function(reactionName) {
logHelper.beginCapture();
return room.user.react('mbland', reactionName)
.then(function() { return listenerCallbackPromise; })
.then(function() { return listenerResult; })
.then(helpers.resolveNextTick, helpers.rejectNextTick)
.then(logHelper.endCaptureResolve(), logHelper.endCaptureReject());
};
Expand Down Expand Up @@ -240,8 +240,8 @@ describe('Integration test', function() {

response.statusCode = 500;
response.payload = payload;
return sendReaction(helpers.REACTION)
.should.be.rejectedWith(errorReply).then(function() {
return sendReaction(helpers.REACTION).should.become(errorReply)
.then(function() {
var logMessages;

room.messages.should.eql([
Expand All @@ -268,10 +268,9 @@ describe('Integration test', function() {
response.payload = { message: 'should not happen' };
});

return sendReaction('sad-face').should.be.rejectedWith(null)
.then(function() {
room.messages.should.eql([['mbland', 'sad-face']]);
logHelper.filteredMessages().should.eql(initLogMessages());
});
return sendReaction('sad-face').should.become(null).then(function() {
room.messages.should.eql([['mbland', 'sad-face']]);
logHelper.filteredMessages().should.eql(initLogMessages());
});
});
});

0 comments on commit 4534793

Please sign in to comment.