| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
These asserts aren't necessary since common.mustCall() already checks that the functions are called exactly the specified number of times (with 1 being the default).
Sorry, something went wrong.
There was a problem hiding this comment.
Instead of using a fixed port as-is like this, consider using a custom http.Agent that returns an error in createConnection(). For an example of this, see this commit. This way we don't end up having a potentially flaky test in the future.
Sorry, something went wrong.
There was a problem hiding this comment.
@mscdex I use not exists host instead.
Sorry, something went wrong.
There was a problem hiding this comment.
assert is unused now.
Sorry, something went wrong.
There was a problem hiding this comment.
These count variables and console.log()s are no longer needed either IMHO.
Sorry, something went wrong.
|
@mscdex @dead-horse Thanks for the quickly review, all changed. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM with a few suggestions.
Sorry, something went wrong.
There was a problem hiding this comment.
I don't think you have to include these first 21 lines in new files.
Sorry, something went wrong.
There was a problem hiding this comment.
const req = http.get({ host }); should fit comfortably on one line.
Sorry, something went wrong.
There was a problem hiding this comment.
You should probably verify that the error received here is the same one that is thrown. If you declare the error outside of the req.on('error', ...) block, but still throw it from in there, then you can just use assert.strictEqual() in here.
Sorry, something went wrong.
Should set req.socket._hadError to true before emit the error event.
Sorry, something went wrong.
Sorry, something went wrong.
|
@mscdex need to restart ci task. |
Sorry, something went wrong.
Sorry, something went wrong.
|
Any idea on the semver-iness of this change? I'd prefer it not to be major but it may have to be. Thoughts? |
Sorry, something went wrong.
|
I'd prefer patch I think. |
Sorry, something went wrong.
|
@jasnell I think this change is a bugfix. |
Sorry, something went wrong.
|
Works for me |
Sorry, something went wrong.
req.socket._hadError should be set before emitting the error event. PR-URL: #14659 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
| Back | FazBrowse Home | New Git URL |
Should set req.socket._hadError to true before emit the error event.
Checklist
Affected core subsystem(s)
http