Close server when rtm.stopServer() is invoked #26
Conversation
|
Thanks for finding the issue and submitting the PR! This looks like the right solution, I added a few comments to tighten things up, but this looks like a great start! |
src/mocker/rtm.js
Outdated
| @@ -85,6 +85,7 @@ rtm.stopServer = function (token) { | |||
| return new Promise((resolve, reject) => { | |||
| const wss = wssServers.get(token) | |||
| if (!wss) { | |||
| server.close() | |||
There was a problem hiding this comment.
server.close is async, we should resolveinside a callback to server.close. The callback will be invoked when all connections are closed. According to the docs:
[server.close] will be called with an Error as its only argument if the server was not open when it was closed.
so I think it's safe to always resolve regardless of errors.
Something like return server.close(() => resolve()) should be fine
src/mocker/rtm.js
Outdated
| @@ -96,6 +97,7 @@ rtm.stopServer = function (token) { | |||
|
|
|||
| logger.debug(`server ${token} closed`) | |||
| wssServers.delete(token) | |||
| server.close() | |||
There was a problem hiding this comment.
Same comment as above about resolving as a callback.
test/mocker/rtm.spec.js
Outdated
|
|
||
| expressAppMock = { | ||
| listen: sinon.stub().returns(serverMock) | ||
| listen: sinon.stub().returns(serverMock), | ||
| close: serverMock.close() |
There was a problem hiding this comment.
You shouldn't need this mock, close is only called on the server returned by listen, not on the expressApp.
test/mocker/rtm.spec.js
Outdated
| @@ -33,10 +33,13 @@ describe('mocker: rtm', function () { | |||
| Server: sinon.stub() | |||
| } | |||
|
|
|||
| serverMock = 'server' | |||
| serverMock = { | |||
| close: sinon.stub().returns() | |||
There was a problem hiding this comment.
let's add some assertions to make sure this is called correctly. You'll need to use yields since it is an async function invoking a callback.
|
Ah yes, I missed that As for your other comments, I've never really used Mocha or Sinon, and you can tell that my solution was a bit rudimental :) Anyway, thanks for the feedback, and I'm glad my solution helped a little bit :) |
|
Ok, I've had another go, please let me know if you have any more feedback for improving things. Thanks! |
|
closes #25 |
Fixes issue #25.
I’m not sure this is the most appropriate way to fix the issue, but it does the job!