Conversation
jsdelfino
left a comment
jsdelfino
left a comment
There was a problem hiding this comment.
Thanks for this pull request. I have a few review comments.
| log('Send result %d, %o', res.statusCode, res.body); | ||
| } | ||
| catch(err) { | ||
| log('Error sending message %o', err); |
There was a problem hiding this comment.
Looks like this code silently discards the caught exception. I think it should be re-thrown.
src/app.js
Outdated
| }] | ||
| } | ||
| }); | ||
| log('Send result %d, %o', res.statusCode, res.body); |
There was a problem hiding this comment.
An exception should be thrown is statusCode is !== 200.
src/app.js
Outdated
| const app = await webapp( | ||
| env.ECHO_APP_ID, env.ECHO_APP_SECRET, | ||
| env.ECHO_WEBHOOK_SECRET); | ||
| cb(null); |
There was a problem hiding this comment.
I don't think there's a need for a callback here as this function was converted to async.
src/app.js
Outdated
| https.createServer(conf, app).listen(port, cb); | ||
| }); | ||
| }); | ||
| const main = async (argv, env, cb) => { |
There was a problem hiding this comment.
The cb parameter is not needed here as the function was converted to async.
src/app.js
Outdated
| } | ||
| } | ||
| catch(err) { | ||
| cb(err); |
There was a problem hiding this comment.
Same comment as above, no need to call a callback here, since the function is async.
src/test/test.js
Outdated
|
|
||
| check(); | ||
| echo.webapp('testappid', 'testsecret', 'testwsecret') | ||
| .then((app) => { |
There was a problem hiding this comment.
I'd suggest to use await here instead of a promise (since this pull request is about using async/await)
src/test/test.js
Outdated
| }); | ||
|
|
||
| it('handles Webhook challenge requests', (done) => { | ||
| it('handles Webhook challenge requests', async (done) => { |
There was a problem hiding this comment.
I don't think using a done callback like this is actually compatible with an async test function.
src/test/test.js
Outdated
| echo.webapp('testappid', 'testsecret', 'testwsecret', (err, app) => { | ||
| expect(err).to.equal(null); | ||
| echo.webapp('testappid', 'testsecret', 'testwsecret') | ||
| .then((app) => { |
There was a problem hiding this comment.
Suggest to convert to async/await here as well.
src/test/test.js
Outdated
| // Expect the request to be rejected | ||
| expect(val.statusCode).to.equal(401); | ||
| echo.webapp('testappid', 'testsecret', 'testwsecret') | ||
| .then((app) => { |
There was a problem hiding this comment.
Suggest to convert to async/await.
src/test/test.js
Outdated
| const server = app.listen(0); | ||
|
|
||
| // Post a chat message to the app | ||
| post('http://localhost:' + server.address().port + '/echo', { |
There was a problem hiding this comment.
Looks like this call wasn't converted to async/await.
2 participants
Hey Sebastian,
I refactored the both the code and the unit tests a bit for async/await with promises. Both end to end and unit tests are passing. Ill send you a screenshot of the echo from my Example space.