test: refactor test-dgram-setBroadcast.js - #11252
Conversation
Sorry, something went wrong.
This test wasn't actually working, as sockets were being closed, allowing the test to exit before any assertions were actually run. This commit refactors the test to maintain the same intended semantics.
|
I don't think test-dgram-membership is afflicted with the same issue, but is it worth refactoring out its identical setup() anyway as a precaution or anything? (I have no opinion either way.) |
Sorry, something went wrong.
Trott
left a comment
There was a problem hiding this comment.
LGTM if CI is green
Sorry, something went wrong.
|
@Trott it looks like test-dgram-membership is fine. I'll probably update it a little bit to at least improve the error message checking with assert.throws(). The problem with this test isn't setup(). The problem here is that the test would synchronously call bind() and then teardown the socket. At least on my machine, this means that nothing was keeping the event loop open, and the code inside the bind() callback was never getting a chance to execute. This seems to be the case on the machine where code coverage was running as well. |
Sorry, something went wrong.
Sorry, something went wrong.
This test wasn't actually working, as sockets were being closed, allowing the test to exit before any assertions were actually run. This commit refactors the test to maintain the same intended semantics. PR-URL: #11252 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This test wasn't actually working, as sockets were being closed, allowing the test to exit before any assertions were actually run. This commit refactors the test to maintain the same intended semantics. PR-URL: #11252 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This test wasn't actually working, as sockets were being closed, allowing the test to exit before any assertions were actually run. This commit refactors the test to maintain the same intended semantics. PR-URL: nodejs#11252 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This test wasn't actually working, as sockets were being closed, allowing the test to exit before any assertions were actually run. This commit refactors the test to maintain the same intended semantics. PR-URL: nodejs#11252 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This test wasn't actually working, as sockets were being closed, allowing the test to exit before any assertions were actually run. This commit refactors the test to maintain the same intended semantics. PR-URL: #11252 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
would need a backport PR to land in v4 |
Sorry, something went wrong.
This test wasn't actually working, as sockets were being closed, allowing the test to exit before any assertions were actually run. This commit refactors the test to maintain the same intended semantics. PR-URL: #11252 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This test wasn't actually working, as sockets were being closed, allowing the test to exit before any assertions were actually run. This commit refactors the test to maintain the same intended semantics.
If anyone is curious, I found this while looking at the code coverage report.
Checklist
Affected core subsystem(s)
test