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

test: add missing assert.throws() #15519

Merged
merged 1 commit into from Oct 25, 2017

Conversation

Projects
None yet
10 participants
@cjihrig
Contributor

cjihrig commented Sep 21, 2017

This commit replaces a try...catch in test/parallel/test-dgram-multicast-set-interface.js with assert.throws(). The existing implementation wasn't actually verifying that a synchronous exception is thrown.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@mscdex mscdex added the dgram label Sep 21, 2017

@lpinca

lpinca approved these changes Sep 21, 2017

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Sep 22, 2017

Contributor

Now that this assertion is actually doing something, the results are all over the place. There are a ton of failures, and I've gone through most of them. The various outcomes are:

  • No error is thrown
  • EINVAL is thrown (what is expected in this PR)
  • ENOPROTOOPT is thrown

Do we want to try to handle all of the cases in this test?

cc: @mcollina and @lostnet (the original author)

EDIT: CI run so others can look at the results: https://ci.nodejs.org/job/node-test-pull-request/10200/

Contributor

cjihrig commented Sep 22, 2017

Now that this assertion is actually doing something, the results are all over the place. There are a ton of failures, and I've gone through most of them. The various outcomes are:

  • No error is thrown
  • EINVAL is thrown (what is expected in this PR)
  • ENOPROTOOPT is thrown

Do we want to try to handle all of the cases in this test?

cc: @mcollina and @lostnet (the original author)

EDIT: CI run so others can look at the results: https://ci.nodejs.org/job/node-test-pull-request/10200/

@lostnet

This comment has been minimized.

Show comment
Hide comment
@lostnet

lostnet Sep 22, 2017

Contributor

@cjihrig I think that my original intention really was just to record which result occurs as this passes libuv's guards (which have consistent EINVAL for some of the other checks) as valid v6:
uv_ip6_addr
getting to do a v6 option attempt:
IPPROTO_IPV6
while the socket is v4 so it was intended to be compatible with:
IPPROTO_IP

I think an OS can view IPPROTO_IPV6/IPV6_MULTICAST_IF as nonsensical (EINVAL) on a IPPROTO_IP socket, sensical but inappropriate (ENOPROTOOPT) or perfectly ok, i.e.: depending on whether it is treating v6 as a separate stack, a mode of a v4/v6 integrated stack or the primary stack that just has options to emulate v4 and restrict its network behavior to a v4 legacy mode.

But I would like to review the OS/configs that pass and that return EINVAL to make sure that they aren't occurring for the wrong reasons as there might be something to fix in libuv or a need to add guards in node to block some calls into libuv.

Contributor

lostnet commented Sep 22, 2017

@cjihrig I think that my original intention really was just to record which result occurs as this passes libuv's guards (which have consistent EINVAL for some of the other checks) as valid v6:
uv_ip6_addr
getting to do a v6 option attempt:
IPPROTO_IPV6
while the socket is v4 so it was intended to be compatible with:
IPPROTO_IP

I think an OS can view IPPROTO_IPV6/IPV6_MULTICAST_IF as nonsensical (EINVAL) on a IPPROTO_IP socket, sensical but inappropriate (ENOPROTOOPT) or perfectly ok, i.e.: depending on whether it is treating v6 as a separate stack, a mode of a v4/v6 integrated stack or the primary stack that just has options to emulate v4 and restrict its network behavior to a v4 legacy mode.

But I would like to review the OS/configs that pass and that return EINVAL to make sure that they aren't occurring for the wrong reasons as there might be something to fix in libuv or a need to add guards in node to block some calls into libuv.

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Sep 26, 2017

Contributor

@lostnet if you need to see which systems had which results, you can browse the CI results in https://ci.nodejs.org/job/node-test-commit/12524/. If you need more information on the system specifics, someone from the build group can probably help you.

cc: @Trott since tests and build are in their wheelhouse.

Contributor

cjihrig commented Sep 26, 2017

@lostnet if you need to see which systems had which results, you can browse the CI results in https://ci.nodejs.org/job/node-test-commit/12524/. If you need more information on the system specifics, someone from the build group can probably help you.

cc: @Trott since tests and build are in their wheelhouse.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 26, 2017

Member

cc: @Trott since tests and build are in their wheelhouse.

Yikes. Maybe just remove the code entirely since it was never meant to actually test anything.

Member

Trott commented Sep 26, 2017

cc: @Trott since tests and build are in their wheelhouse.

Yikes. Maybe just remove the code entirely since it was never meant to actually test anything.

@lostnet

This comment has been minimized.

Show comment
Hide comment
@lostnet

lostnet Sep 27, 2017

Contributor

@Trott and @cjihrig It is definitely a poorly handled edge case for OSes but none of the results look like they put libuv/node in an inconsistent state.

It looks like either real or doc bugs in aix/windows that they accept AF_INET6 options on AF_INET sockets, but I don't see that they create any problems for libuv/node's later management of the socket.

For Linux it looks like it prefers ENOPROTOOPT throughout socket code (only partially remapping it in a compat mode) so its not indicating failure was on the incorrect check.

Contributor

lostnet commented Sep 27, 2017

@Trott and @cjihrig It is definitely a poorly handled edge case for OSes but none of the results look like they put libuv/node in an inconsistent state.

It looks like either real or doc bugs in aix/windows that they accept AF_INET6 options on AF_INET sockets, but I don't see that they create any problems for libuv/node's later management of the socket.

For Linux it looks like it prefers ENOPROTOOPT throughout socket code (only partially remapping it in a compat mode) so its not indicating failure was on the incorrect check.

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Sep 27, 2017

Member

I had a look at the tests and it seems like other tests could be improved as well.
The current fifth test is checking for /Error/ (line 66) but that could be any error type. So we should make sure the RegExp is corrected to /^Error/. The fourth tests for /TypeError/. That is also not ideal and could be improved (we might just use common.expectsError instead of using the RegExp).

Member

BridgeAR commented Sep 27, 2017

I had a look at the tests and it seems like other tests could be improved as well.
The current fifth test is checking for /Error/ (line 66) but that could be any error type. So we should make sure the RegExp is corrected to /^Error/. The fourth tests for /TypeError/. That is also not ideal and could be improved (we might just use common.expectsError instead of using the RegExp).

@lostnet

This comment has been minimized.

Show comment
Hide comment
@lostnet

lostnet Sep 28, 2017

Contributor

@BridgeAR It should be possible to be more precise on the /TypeError/ but on the general /Error/ having a regex was just added to pass newer lint/style checks than the tests. With the new error system it might make sense to test these have an errno set by UV now, but which error to expect it to pass back up is undefined.

Contributor

lostnet commented Sep 28, 2017

@BridgeAR It should be possible to be more precise on the /TypeError/ but on the general /Error/ having a regex was just added to pass newer lint/style checks than the tests. With the new error system it might make sense to test these have an errno set by UV now, but which error to expect it to pass back up is undefined.

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Oct 1, 2017

Member

@lostnet I do not think that it is a good idea to just expect "any error". If something throws it should be reliable, so we should test for a specific error. If that is not the case, there might be a bug or we should work on it that it becomes reliable.

More important for me right now is though how we progress here. I would really like to fix the uncovered issue.

Member

BridgeAR commented Oct 1, 2017

@lostnet I do not think that it is a good idea to just expect "any error". If something throws it should be reliable, so we should test for a specific error. If that is not the case, there might be a bug or we should work on it that it becomes reliable.

More important for me right now is though how we progress here. I would really like to fix the uncovered issue.

@lostnet

This comment has been minimized.

Show comment
Hide comment
@lostnet

lostnet Oct 2, 2017

Contributor

@BridgeAR Its possible to find libuv dumbing down to a lowest common platform for common interfaces, so it might be consistent to do so.. But it is really an antipattern to always return EINVAL just because at least one platform will and it is difficult to converge on useful errors across the platforms that are trying to be more helpful.

The normal pattern in network programming is to look for any error and just print it in compatible code (which is already more helpful if it isn't always EINVAL) and attempt to address what you can in platform specific code for platforms that use distinguishable errors.

I don't really see this as an uncovered problem as the docs were clear on being vague, and I think that is important for programmers who want the choice to do native-like integration with their specific platform(s) without interference.

Contributor

lostnet commented Oct 2, 2017

@BridgeAR Its possible to find libuv dumbing down to a lowest common platform for common interfaces, so it might be consistent to do so.. But it is really an antipattern to always return EINVAL just because at least one platform will and it is difficult to converge on useful errors across the platforms that are trying to be more helpful.

The normal pattern in network programming is to look for any error and just print it in compatible code (which is already more helpful if it isn't always EINVAL) and attempt to address what you can in platform specific code for platforms that use distinguishable errors.

I don't really see this as an uncovered problem as the docs were clear on being vague, and I think that is important for programmers who want the choice to do native-like integration with their specific platform(s) without interference.

cjihrig added a commit to cjihrig/node-1 that referenced this pull request Oct 24, 2017

test: add missing assertion
This commit adds an assertion to an existing try...catch
statement. Unfortunately, assert.throws() cannot be used
because the operation succeeds on some platforms, throws
EINVAL on some platforms, and throws ENOPROTOOPT on
others.

PR-URL: nodejs#15519
@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Oct 24, 2017

Contributor

I've decided to try reverting to a try...catch and detecting the appropriate errors in the catch block.

CI: https://ci.nodejs.org/job/node-test-pull-request/10931/

Contributor

cjihrig commented Oct 24, 2017

I've decided to try reverting to a try...catch and detecting the appropriate errors in the catch block.

CI: https://ci.nodejs.org/job/node-test-pull-request/10931/

test: add missing assertion
This commit adds an assertion to an existing try...catch
statement. Unfortunately, assert.throws() cannot be used
because the operation succeeds on some platforms, throws
EINVAL on some platforms, and throws ENOPROTOOPT on
others.

PR-URL: #15519
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@cjihrig cjihrig merged commit c5e3353 into nodejs:master Oct 25, 2017

@cjihrig cjihrig deleted the cjihrig:test branch Oct 25, 2017

cjihrig added a commit that referenced this pull request Oct 25, 2017

test: add missing assertion
This commit adds an assertion to an existing try...catch
statement. Unfortunately, assert.throws() cannot be used
because the operation succeeds on some platforms, throws
EINVAL on some platforms, and throws ENOPROTOOPT on
others.

PR-URL: #15519
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@cjihrig cjihrig referenced this pull request Oct 25, 2017

Merged

v8.8.1 proposal #16498

addaleax added a commit to ayojs/ayo that referenced this pull request Oct 26, 2017

test: add missing assertion
This commit adds an assertion to an existing try...catch
statement. Unfortunately, assert.throws() cannot be used
because the operation succeeds on some platforms, throws
EINVAL on some platforms, and throws ENOPROTOOPT on
others.

PR-URL: nodejs/node#15519
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017

test: add missing assertion
This commit adds an assertion to an existing try...catch
statement. Unfortunately, assert.throws() cannot be used
because the operation succeeds on some platforms, throws
EINVAL on some platforms, and throws ENOPROTOOPT on
others.

PR-URL: nodejs/node#15519
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment