Skip to content

cctest: Fix cctest failure on windows#14111

Closed
MSLaguana wants to merge 1 commit into
nodejs:masterfrom
MSLaguana:patch-1
Closed

cctest: Fix cctest failure on windows#14111
MSLaguana wants to merge 1 commit into
nodejs:masterfrom
MSLaguana:patch-1

Conversation

@MSLaguana
Copy link
Copy Markdown
Contributor

@MSLaguana MSLaguana commented Jul 6, 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.

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

cctest
@MSLaguana
cctest: Fix cctest failure on windows
a2be5d1 
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-bot nodejs-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
Copy Markdown
Contributor Author

MSLaguana commented Jul 6, 2017

@eugeneo Can you take a look please?
@refack
Copy link
Copy Markdown
Contributor

refack commented Jul 6, 2017

/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 refack mentioned this pull request Jul 6, 2017
Closed
@refack
Copy link
Copy Markdown
Contributor

refack commented Jul 6, 2017

CI: https://ci.nodejs.org/job/node-test-commit/10993/
eugeneo
eugeneo approved these changes Jul 6, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@eugeneo eugeneo 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.
@mscdex mscdex 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
Copy Markdown
Contributor

refack commented Jul 6, 2017

@Trott look it's green, not yellow. 🌞 🍹
@kfarnung kfarnung mentioned this pull request Jul 6, 2017
Merged
2 tasks
@refack
Copy link
Copy Markdown
Contributor

refack commented Jul 7, 2017

@joyeecheung does this work for you (before and/or after)?
@refack
Copy link
Copy Markdown
Contributor

refack commented Jul 7, 2017

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
joaocgreis
joaocgreis approved these changes Jul 9, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@joaocgreis joaocgreis 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
Copy Markdown
Member

kunalspathak commented Jul 9, 2017

@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
@MSLaguana @refack
test: fix cctest failure on Windows
94dd425 
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 <refack@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Kunal Pathak <kunal.pathak@microsoft.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack
Copy link
Copy Markdown
Contributor

refack commented Jul 10, 2017

Landed in 94dd425
@refack refack closed this Jul 10, 2017
@MSLaguana MSLaguana deleted the patch-1 branch July 10, 2017 16:37
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@MSLaguana @addaleax
test: fix cctest failure on Windows
de3cc38 
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 <refack@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Kunal Pathak <kunal.pathak@microsoft.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Jul 11, 2017
Merged
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@MSLaguana @addaleax
test: fix cctest failure on Windows
d0e4e2b 
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 <refack@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Kunal Pathak <kunal.pathak@microsoft.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@MSLaguana @Fishrock123
test: fix cctest failure on Windows
65ab656 
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 <refack@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Kunal Pathak <kunal.pathak@microsoft.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bzoz bzoz mentioned this pull request Aug 17, 2017
Closed
2 tasks
@refack refack mentioned this pull request Aug 17, 2017
Closed
@bzoz bzoz mentioned this pull request Aug 31, 2017
Closed
Sign up for free to 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++. inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.