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.

Sign up for GitHub

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

Jump to bottom

cctest: Fix cctest failure on windows #14111

Closed
wants to merge 1 commit into from
Closed

cctest: Fix cctest failure on windows #14111

wants to merge 1 commit into from

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

@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
Contributor Author

@eugeneo Can you take a look please?

@refack
Copy link
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
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
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
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
Contributor

refack commented Jul 7, 2017

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

@refack
Copy link
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
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
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
@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
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
Reviewers

@kunalspathak kunalspathak kunalspathak approved these changes

@cjihrig cjihrig cjihrig approved these changes

@kfarnung kfarnung kfarnung approved these changes

@jasnell jasnell jasnell approved these changes

@eugeneo eugeneo eugeneo approved these changes

@joaocgreis joaocgreis joaocgreis approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.