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

Updates for Node v6.6.0+ UnhandledPromiseRejectionWarning #6

Merged
merged 2 commits into from
Apr 28, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 33 additions & 25 deletions _pages/components/middleware.md
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,8 @@ moving on to the next section.

Now that `findMatchingRule` is in place, let's return to `execute` and begin
to test it. As a base case, make sure that `execute` calls `next(done)` and
returns if no rule matches:
returns an empty rejected `Promise` if no rule matches, to signify that no
action was taken and no error occurred:

```js
Middleware.prototype.execute = function(context, next, done) {
Expand All @@ -497,7 +498,8 @@ Middleware.prototype.execute = function(context, next, done) {
rule = this.findMatchingRule(message);

if (!rule) {
return next(done);
next(done);
return Promise.reject();
}
return 'not yet implemented';
};
Expand Down Expand Up @@ -575,8 +577,10 @@ parse a message and file an issue` case:
```js
it('should ignore messages that do not match', function() {
delete context.response.message.rawMessage;
expect(middleware.execute(context, next, hubotDone)).to.be.undefined;
next.calledWith(hubotDone).should.be.true;
middleware.execute(context, next, hubotDone)
.should.be.rejectedWith(undefined).then(function() {;
next.calledWith(hubotDone).should.be.true;
});
});
```

Expand Down Expand Up @@ -643,7 +647,8 @@ Middleware.prototype.execute = function(context, next, done) {
msgId;

if (!rule) {
return next(done);
next(done);
return Promise.reject();
}

msgId = messageId(message);
Expand Down Expand Up @@ -1413,18 +1418,18 @@ should return `undefined`:

```js
it('should not file another issue for the same message when ' +
'one is in progress', function(done) {
'one is in progress', function() {
var result;

result = middleware.execute(context, next, hubotDone);
if (middleware.execute(context, next, hubotDone) !== undefined) {
return done(new Error('middleware.execute did not prevent filing a ' +
'second issue when one was already in progress'));
}

result.should.become(helpers.ISSUE_URL).then(function() {
logger.info.args.should.contain(helpers.logArgs('already in progress'));
});
return middleware.execute(context, next, hubotDone)
.should.be.rejectedWith(undefined).then(function() {
return result.should.become(helpers.ISSUE_URL).then(function() {
logger.info.args.should.include.something.that.deep.equals(
helpers.logArgs('already in progress'));
next.calledWith(hubotDone).should.be.true;
});
});
});
```

Expand Down Expand Up @@ -1493,7 +1498,8 @@ and add the message ID as a property inside `execute`:
msgId = messageId(message);
if (this.inProgress[msgId]) {
this.logger(msgId, 'already in progress');
return next(done);
next(done);
return Promise.reject();
}
this.inProgress[msgId] = true;
```
Expand Down Expand Up @@ -1583,16 +1589,17 @@ one more `middleware.execute` call _inside the callback_, and return the
result of the assertion:

```js
result.should.become(helpers.ISSUE_URL).then(function() {
logger.info.args.should.include.something.that.deep.equals(
helpers.logArgs('already in progress'));
return result.should.become(helpers.ISSUE_URL).then(function() {
logger.info.args.should.include.something.that.deep.equals(
helpers.logArgs('already in progress'));
next.calledWith(hubotDone).should.be.true;

// Make another call to ensure that the ID is cleaned up. Normally the
// message will have a successReaction after the first successful
// request, but we'll test that in another case.
return middleware.execute(context, next, hubotDone)
.should.become(helpers.ISSUE_URL);
});
// Make another call to ensure that the ID is cleaned up. Normally
// the message will have a successReaction after the first
// successful request, but we'll test that in another case.
return middleware.execute(context, next, hubotDone)
.should.become(helpers.ISSUE_URL);
});
```

Again, in frameworks other than Mocha that don't have native `Promise`
Expand Down Expand Up @@ -2086,7 +2093,8 @@ Middleware.prototype.execute = function(context, next, done) {
JSON.stringify(context.response.message.rawMessage, null, 2);
this.logger.error(null, errorMessage);
context.response.reply(errorMessage);
return next(done);
next(done);
return Promise.reject();
}
};
```
Expand Down
12 changes: 9 additions & 3 deletions _pages/testing-component-integration.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,9 @@ module.exports = function(robot) {
logger);

robot.receiveMiddleware(function(context, next, done) {
impl.execute(context, next, done);
// Catch rejected Promises to avoid the UnhandledPromiseRejectionWarning
// in Node.js v6.6.0 and higher.
impl.execute(context, next, done).catch(function() { });
});
logger.info(null, 'registered receiveMiddleware');
};
Expand Down Expand Up @@ -703,7 +705,9 @@ module.exports = function(robot) {
logger);

robot.receiveMiddleware(function(context, next, done) {
impl.execute(context, next, done);
// Catch rejected Promises to avoid the UnhandledPromiseRejectionWarning
// in Node.js v6.6.0 and higher.
impl.execute(context, next, done).catch(function() { });
});
logger.info(null, 'registered receiveMiddleware');

Expand Down Expand Up @@ -976,7 +980,9 @@ module.exports = function(robot) {
logger);

middleware = function(context, next, done) {
impl.execute(context, next, done);
// Catch rejected Promises to avoid the UnhandledPromiseRejectionWarning
// in Node.js v6.6.0 and higher.
impl.execute(context, next, done).catch(function() { });
};
middleware.impl = impl;
robot.receiveMiddleware(middleware);
Expand Down
9 changes: 6 additions & 3 deletions solutions/05-middleware/lib/middleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ Middleware.prototype.execute = function(context, next, done) {
JSON.stringify(context.response.message.rawMessage, null, 2);
this.logger.error(null, errorMessage);
context.response.reply(errorMessage);
return next(done);
next(done);
return Promise.reject();
}
};

Expand All @@ -40,13 +41,15 @@ function doExecute(middleware, context, next, done) {
finish;

if (!rule) {
return next(done);
next(done);
return Promise.reject();
}

msgId = messageId(message);
if (middleware.inProgress[msgId]) {
middleware.logger.info(msgId, 'already in progress');
return next(done);
next(done);
return Promise.reject();
}
middleware.inProgress[msgId] = true;

Expand Down
37 changes: 20 additions & 17 deletions solutions/05-middleware/test/middleware-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,29 +142,31 @@ describe('Middleware', function() {

it('should ignore messages that do not match', function() {
delete context.response.message.rawMessage;
expect(middleware.execute(context, next, hubotDone)).to.be.undefined;
next.calledWith(hubotDone).should.be.true;
middleware.execute(context, next, hubotDone)
.should.be.rejectedWith(undefined).then(function() {
next.calledWith(hubotDone).should.be.true;
});
});

it('should not file another issue for the same message when ' +
'one is in progress', function() {
var result;

result = middleware.execute(context, next, hubotDone);
expect(middleware.execute(context, next, hubotDone)).to.eql(undefined,
'middleware.execute did not prevent filing a second issue ' +
'when one was already in progress');

return result.should.become(helpers.ISSUE_URL).then(function() {
logger.info.args.should.include.something.that.deep.equals(
helpers.logArgs('already in progress'));

// Make another call to ensure that the ID is cleaned up. Normally the
// message will have a successReaction after the first successful
// request, but we'll test that in another case.
return middleware.execute(context, next, hubotDone)
.should.become(helpers.ISSUE_URL);
});
return middleware.execute(context, next, hubotDone)
.should.be.rejectedWith(undefined).then(function() {
return result.should.become(helpers.ISSUE_URL).then(function() {
logger.info.args.should.include.something.that.deep.equals(
helpers.logArgs('already in progress'));
next.calledWith(hubotDone).should.be.true;

// Make another call to ensure that the ID is cleaned up. Normally
// the message will have a successReaction after the first
// successful request, but we'll test that in another case.
return middleware.execute(context, next, hubotDone)
.should.become(helpers.ISSUE_URL);
});
});
});

it('should not file another issue for the same message when ' +
Expand Down Expand Up @@ -250,7 +252,8 @@ describe('Middleware', function() {
JSON.stringify(helpers.reactionAddedMessage(), null, 2);

slackClient.getChannelName.throws();
expect(middleware.execute(context, next, hubotDone)).to.be.undefined;
middleware.execute(context, next, hubotDone)
.should.be.rejectedWith(undefined);
next.calledWith(hubotDone).should.be.true;
context.response.reply.args.should.eql([[errorMessage]]);
logger.error.args.should.eql([[null, errorMessage]]);
Expand Down
9 changes: 6 additions & 3 deletions solutions/06-integration/lib/middleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ Middleware.prototype.execute = function(context, next, done) {
JSON.stringify(context.response.message.rawMessage, null, 2);
this.logger.error(null, errorMessage);
context.response.reply(errorMessage);
return next(done);
next(done);
return Promise.reject();
}
};

Expand All @@ -40,13 +41,15 @@ function doExecute(middleware, context, next, done) {
finish;

if (!rule) {
return next(done);
next(done);
return Promise.reject();
}

msgId = messageId(message);
if (middleware.inProgress[msgId]) {
middleware.logger.info(msgId, 'already in progress');
return next(done);
next(done);
return Promise.reject();
}
middleware.inProgress[msgId] = true;

Expand Down
4 changes: 3 additions & 1 deletion solutions/06-integration/scripts/slack-github-issues.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ module.exports = function(robot) {
logger);

middleware = function(context, next, done) {
impl.execute(context, next, done);
// Catch rejected Promises to avoid the UnhandledPromiseRejectionWarning
// in Node.js v6.6.0 and higher.
impl.execute(context, next, done).catch(function() { });
};
middleware.impl = impl;
robot.receiveMiddleware(middleware);
Expand Down
9 changes: 6 additions & 3 deletions solutions/07-system/lib/middleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ Middleware.prototype.execute = function(context, next, done) {
JSON.stringify(context.response.message.rawMessage, null, 2);
this.logger.error(null, errorMessage);
context.response.reply(errorMessage);
return next(done);
next(done);
return Promise.reject();
}
};

Expand All @@ -40,13 +41,15 @@ function doExecute(middleware, context, next, done) {
finish;

if (!rule) {
return next(done);
next(done);
return Promise.reject();
}

msgId = messageId(message);
if (middleware.inProgress[msgId]) {
middleware.logger.info(msgId, 'already in progress');
return next(done);
next(done);
return Promise.reject();
}
middleware.inProgress[msgId] = true;

Expand Down
4 changes: 3 additions & 1 deletion solutions/07-system/scripts/slack-github-issues.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ module.exports = function(robot) {
logger);

middleware = function(context, next, done) {
impl.execute(context, next, done);
// Catch rejected Promises to avoid the UnhandledPromiseRejectionWarning
// in Node.js v6.6.0 and higher.
impl.execute(context, next, done).catch(function() { });
};
middleware.impl = impl;
robot.receiveMiddleware(middleware);
Expand Down
9 changes: 6 additions & 3 deletions solutions/complete/lib/middleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ Middleware.prototype.execute = function(context, next, done) {
JSON.stringify(context.response.message.rawMessage, null, 2);
this.logger.error(null, errorMessage);
context.response.reply(errorMessage);
return next(done);
next(done);
return Promise.reject();
}
};

Expand All @@ -40,13 +41,15 @@ function doExecute(middleware, context, next, done) {
finish;

if (!rule) {
return next(done);
next(done);
return Promise.reject();
}

msgId = messageId(message);
if (middleware.inProgress[msgId]) {
middleware.logger.info(msgId, 'already in progress');
return next(done);
next(done);
return Promise.reject();
}
middleware.inProgress[msgId] = true;

Expand Down
4 changes: 3 additions & 1 deletion solutions/complete/scripts/slack-github-issues.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ module.exports = function(robot) {
logger);

middleware = function(context, next, done) {
impl.execute(context, next, done);
// Catch rejected Promises to avoid the UnhandledPromiseRejectionWarning
// in Node.js v6.6.0 and higher.
impl.execute(context, next, done).catch(function() { });
};
middleware.impl = impl;
robot.receiveMiddleware(middleware);
Expand Down
Loading