Skip to content

Conversation

@MSLaguana
Copy link
Contributor

Linux converts the ipv6 address "::" to "::1", while windows does not.
By explicitly using "::1" in the test we allow it to succeed on windows.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

cctest

Linux converts the ipv6 address "::" to "::1", while windows does not. By explicitly using "::1" in the test we allow it to succeed on windows.
@nodejs-github-botnodejs-github-bot added dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests. labels Jul 6, 2017
@MSLaguana
Copy link
ContributorAuthor

@eugeneo Can you take a look please?

@refack
Copy link
Contributor

/cc @nodejs/build Ref: nodejs/build#769

CI is false Green

[ RUN ] InspectorSocketServerTest.BindsToIpV6 test\cctest\test_inspector_socket_server.cc(265): error: Value of: status Actual: -4090 Expected: 0 Unable to connect: address not available c:\workspace\node-test-binary-windows\RUN_SUBSET\0\VS_VERSION\vcbt2015\label\win10\Release\cctest.exe: test\cctest\test_inspector_socket_server.cc:247: Assertion `(err) == (0)' failed. 

https://ci.nodejs.org/job/node-test-binary-windows/9659/RUN_SUBSET=0,VS_VERSION=vcbt2015,label=win10/

@refack
Copy link
Contributor

Copy link
Contributor

@eugeneoeugeneo left a comment

Choose a reason for hiding this comment

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

LGTM if it passes CI on all platforms.

@mscdexmscdex added c++ Issues and PRs that require attention from people who are familiar with C++. windows Issues and PRs related to the Windows platform. labels Jul 6, 2017
@refack
Copy link
Contributor

@Trott look it's green, not yellow. 🌞 🍹

@refack
Copy link
Contributor

@joyeecheung does this work for you (before and/or after)?

@refack
Copy link
Contributor

Goal achieved ✔️

[ RUN ] InspectorSocketServerTest.BindsToIpV6 [ OK ] InspectorSocketServerTest.BindsToIpV6 (4 ms) [----------] 10 tests from InspectorSocketServerTest (20 ms total) [----------] Global test environment tear-down [==========] 48 tests from 6 test cases ran. (84 ms total) [ PASSED ] 48 tests. 

https://ci.nodejs.org/job/node-test-binary-windows/9674/RUN_SUBSET=0,VS_VERSION=vcbt2015,label=win10/console

Copy link
Member

@joaocgreisjoaocgreis left a comment

Choose a reason for hiding this comment

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

Rubber-stamp LGTM. I will add cctest.tap to the list of tap files in CI when this lands.

@kunalspathak
Copy link
Member

@joaocgreis , I made minor changes in CI job for cctest.tap but didn't complete all changes because cctest was still failing. If you want, I can complete those changes once this PR lands.

refack pushed a commit to refack/node that referenced this pull request Jul 10, 2017
Linux converts the ipv6 address "::" to "::1", while windows does not. By explicitly using "::1" in the test we allow it to succeed on windows. PR-URL: nodejs#14111 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Kunal Pathak <[email protected]> Reviewed-By: João Reis <[email protected]> Reviewed-By: James M Snell <[email protected]>
@refack
Copy link
Contributor

Landed in 94dd425

@refackrefack closed this Jul 10, 2017
@MSLaguanaMSLaguana deleted the patch-1 branch July 10, 2017 16:37
addaleax pushed a commit that referenced this pull request Jul 11, 2017
Linux converts the ipv6 address "::" to "::1", while windows does not. By explicitly using "::1" in the test we allow it to succeed on windows. PR-URL: #14111 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Kunal Pathak <[email protected]> Reviewed-By: João Reis <[email protected]> Reviewed-By: James M Snell <[email protected]>
@addaleaxaddaleax mentioned this pull request Jul 11, 2017
addaleax pushed a commit that referenced this pull request Jul 18, 2017
Linux converts the ipv6 address "::" to "::1", while windows does not. By explicitly using "::1" in the test we allow it to succeed on windows. PR-URL: #14111 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Kunal Pathak <[email protected]> Reviewed-By: João Reis <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
Linux converts the ipv6 address "::" to "::1", while windows does not. By explicitly using "::1" in the test we allow it to succeed on windows. PR-URL: #14111 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Kunal Pathak <[email protected]> Reviewed-By: João Reis <[email protected]> Reviewed-By: James M Snell <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.inspectorIssues and PRs related to the V8 inspector protocoltestIssues and PRs related to the tests.windowsIssues and PRs related to the Windows platform.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

11 participants

@MSLaguana@refack@kunalspathak@joaocgreis@eugeneo@jasnell@kfarnung@cjihrig@mscdex@MylesBorins@nodejs-github-bot