Skip to content
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: move the corresponding IPv4 and IPv6 tests #28124

Closed
wants to merge 2 commits into from

Conversation

zero1five
Copy link
Contributor

Move the IPv4 and IPv6 test in the isIp file to the corresponding file,
and remove non-string tests(isIP test is only for string input).

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Move the IPv4 and IPv6 test in the isIp file to the corresponding file,
and remove non-string tests(isIP test is only for string input).
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 7, 2019
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zero1five Thanks a lot for your change but I have some concerns with this:

In case of a failure the test would only report e.g. 0 !== 4. It would not tell what ip was used in the specific test. Thus, the usefulness seems to decrease due to the change.

It also removes a couple of tests that do not seem to be duplicated tests. For those reasons I am -1 on this change.

assert.strictEqual(net.isIPv4(null), false);
assert.strictEqual(net.isIPv4(123), false);
assert.strictEqual(net.isIPv4(true), false);
assert.strictEqual(net.isIPv4({}), false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These cases are not tested in the other file. Is there a specific reason for removing them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About this, I think isIPxx since it only supports string input, testing non-string input is an unnecessary test, don't get useful feedback in run the test case.

I'm not sure if it should do this kind of testing in node, imo, It is up to the user to make a simple assertion before using it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not certain I can follow. We verify that the API works as expected and to have a full coverage.

@zero1five
Copy link
Contributor Author

@BridgeAR Yes! I ignored the fact that the error message was not very readable, so I made some changes, and it now looks like this:

assert.js:89
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: 

net.isIPv6('1:') !== true

    at validateIsIPv6 (...)
    at Array.forEach (<anonymous>)
    ...

add error messages to make it easier to read.

@jasnell
Copy link
Member

jasnell commented Jul 7, 2020

Ping @BridgeAR, looks like this needed a follow up review from you.

@aduh95 aduh95 requested a review from BridgeAR October 19, 2020 17:28
@aduh95 aduh95 added review wanted PRs that need reviews. and removed stalled Issues and PRs that are stalled. labels Oct 19, 2020
@aduh95
Copy link
Contributor

aduh95 commented Oct 19, 2020

@zero1five This would need a rebase to solve the conflicts.

@aduh95 aduh95 added the stalled Issues and PRs that are stalled. label Oct 19, 2020
@github-actions
Copy link
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@github-actions
Copy link
Contributor

Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review wanted PRs that need reviews. stalled Issues and PRs that are stalled. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants