-
Notifications
You must be signed in to change notification settings - Fork 113
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
Fix 178 #184
Fix 178 #184
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you think that it's worth to mention this at README?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with @vieiralucas regarding README update
lib/request.js
Outdated
@@ -241,7 +259,7 @@ module.exports.agent = TestAgent; | |||
* @api private | |||
*/ | |||
|
|||
function Test (app, method, path) { | |||
function Test (app, method, path, onFinish) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's onFinish
used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe he was going to use it as a callback, but ended up going with the on('end'
approach.
Anyways it looks like this parameter is not being used and should be removed 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops 😊 yeah my original implementation had onFinish
but then I realised I can use .on
. Will remove.
This automatically closes server connections after making a request; so that test runners (like mocha 4) aren't left hanging after the test execution. If someone really needs to keep the server open, `.keepOpen()` can be used. Fixes #178 BREAKING CHANGE: This change closes servers down after every request. If you want to use the server for reasons at the same time as your test suite or for some other reason you dont want the server to automatically be shut-down, then you'll need to change any `chai.request` callsites to also use the `keepOpen` comand, like so: ```js chai.request(server).keepOpen() ```
README changes added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I won't be able to merge until sometime tomorrow, so ideal if someone else can.
@vieiralucas for another +1 and merge 😄 |
@keithamus can you make the release? |
Yup - few bits to do before we make the release. I'll raise some more PRs shortly |
In preparing for a release I noticed the severity of #178 was affecting our own test suite, so I decided to fix it. The commit description should hopefully provide context: