Skip to content

Conversation

@bnoordhuis
Copy link
Member

@bnoordhuisbnoordhuis commented Aug 9, 2016

@bnoordhuisbnoordhuis added c++ Issues and PRs that require attention from people who are familiar with C++. build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Aug 9, 2016
@bnoordhuis
Copy link
MemberAuthor

Different take that moves merging of the TAP files out of tools/test.py and into a standalone script: https://ci.nodejs.org/job/node-test-pull-request/3592/

Copy link
Member

Choose a reason for hiding this comment

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

Will this work without renumbering the individual tests in the files being merged?
As it stands I think we'll end up with multiple ok 1 ....

@jbergstroem
Copy link
Member

cc @nodejs/build

@joaocgreis
Copy link
Member

On Windows, vcbuild test-ci already runs cctest but ignores the results. Here is the error from a Debug build:

Running main() from gtest_main.cc [==========] Running 22 tests from 2 test cases. [----------] Global test environment set-up. [----------] 4 tests from UtilTest [ RUN ] UtilTest.ListHead [ OK ] UtilTest.ListHead (0 ms) [ RUN ] UtilTest.StringEqualNoCase [ OK ] UtilTest.StringEqualNoCase (0 ms) [ RUN ] UtilTest.StringEqualNoCaseN [ OK ] UtilTest.StringEqualNoCaseN (0 ms) [ RUN ] UtilTest.ToLower [ OK ] UtilTest.ToLower (0 ms) [----------] 4 tests from UtilTest (0 ms total) [----------] 18 tests from InspectorSocketTest [ RUN ] InspectorSocketTest.ReadsAndWritesInspectorMessage Assertion failed: backlog > 0, file src\win\tcp.c, line 561 

@bnoordhuis
Copy link
MemberAuthor

Removed the script again, made changes to vcbuild.bat and added a -Wformat fix for the inspector cctest because I didn't feel like filing a separate PR for that. :-)

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

@ofrobots@eugeneo Running the cctests seems to have unearthed some issues. Several of the buildbots fail like this (from https://ci.nodejs.org/job/node-test-commit-linux/4529/nodes=debian8-x86/console):

# Value of: 0 # Expected: uv_is_active(reinterpret_cast<uv_handle_t*>(&socket)) # Which is: 1 not ok 9 - InspectorSocketTest.ExtraLettersBeforeRequest --- duration_ms: 0.001 ... # Value of: 0 # Expected: uv_is_active(reinterpret_cast<uv_handle_t*>(&socket)) # Which is: 1 not ok 10 - InspectorSocketTest.RequestWithoutKey --- duration_ms: 0 ... # Value of: 0 # Expected: uv_is_active(reinterpret_cast<uv_handle_t*>(&socket)) # Which is: 1 ok 11 - InspectorSocketTest.KillsConnectionOnProtocolViolation 

@bnoordhuis
Copy link
MemberAuthor

Rebased, let's see if the test failures have been resolved: https://ci.nodejs.org/job/node-test-pull-request/3809/

@eugeneo
Copy link
Contributor

I'm working on it - failures are still there...

On Tue, Aug 23, 2016 at 12:10 PM Ben Noordhuis [email protected]
wrote:

Rebased, let's see if the test failures have been resolved:
https://ci.nodejs.org/job/node-test-pull-request/3809/


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8034 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARkrScGJo88I6TjfrW1w9jXvQZkVG6Fks5qi0WggaJpZM4JgM5v
.

@eugeneo
Copy link
Contributor

FYI - the test on Windows hit #8155, I'll create a PR once I fix it.

@bnoordhuis
Copy link
MemberAuthor

@bnoordhuis
Copy link
MemberAuthor

@eugeneo cctest failures still appear to be unfixed:

not ok 8 - InspectorSocketTest.ExtraTextBeforeRequest --- duration_ms: 0 ... # Value of: 0 # Expected: uv_is_active(reinterpret_cast<uv_handle_t*>(&socket)) # Which is: 1 not ok 9 - InspectorSocketTest.ExtraLettersBeforeRequest --- duration_ms: 0 ... # Value of: 0 # Expected: uv_is_active(reinterpret_cast<uv_handle_t*>(&socket)) # Which is: 1 not ok 10 - InspectorSocketTest.RequestWithoutKey --- duration_ms: 0 ... # Value of: 0 # Expected: uv_is_active(reinterpret_cast<uv_handle_t*>(&socket)) # Which is: 1 

@eugeneo
Copy link
Contributor

cctest works for me on Windows, Mac and Linux after I apply #8528

@bnoordhuis
Copy link
MemberAuthor

bnoordhuis commented Sep 14, 2016

CI with fixes from #8505 and #8528 incorporated: https://ci.nodejs.org/job/node-test-pull-request/4044/

EDIT: Green, except for an infrastructure failure on one of the ARM buildbots.

ofrobots pushed a commit that referenced this pull request Sep 16, 2016
Should help with #8034. PR-URL: #8528 Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
@bnoordhuis
Copy link
MemberAuthor

@bnoordhuis
Copy link
MemberAuthor

Quite a few flakes but none related to this PR. Can I get a LGTM?

@bnoordhuisbnoordhuis deleted the gtest-tap branch October 3, 2016 17:49
@bnoordhuisbnoordhuis merged commit b3d283a into nodejs:masterOct 3, 2016
@bnoordhuis
Copy link
MemberAuthor

Back-porters, commit b3d283a (the last one) should be omitted when back-porting to v4.x (but not v6.x.)

@mscdex
Copy link
Contributor

mscdex commented Oct 6, 2016

@bnoordhuis Why did the default result printer switch from pretty print to TAP? I get TAP output from cctest when running make test locally now.

jasnell pushed a commit that referenced this pull request Oct 6, 2016
Teach gtest to produce TAP so we can integrate it better with our CI tooling. TAP is printed to stdout but it can also be written to file by passing the `--gtest_output=tap:filename.tap` switch to cctest. PR-URL: #8034 Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 6, 2016
Enable the cctests on the CI now that they know how to write TAP output. PR-URL: #8034 Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 6, 2016
Print size_t and ssize_t using %zd and %zu respectively, not %ld. PR-URL: #8034 Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
Teach gtest to produce TAP so we can integrate it better with our CI tooling. TAP is printed to stdout but it can also be written to file by passing the `--gtest_output=tap:filename.tap` switch to cctest. PR-URL: #8034 Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
Enable the cctests on the CI now that they know how to write TAP output. PR-URL: #8034 Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
Print size_t and ssize_t using %zd and %zu respectively, not %ld. PR-URL: #8034 Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

waiting to backport this until we have consensus on the tap output... has this been decided on master yet?

@mscdex
Copy link
Contributor

@thealphanerd The default reporter format change was reverted in master.

@MylesBorins
Copy link
Contributor

@mscdex can you give me the PR that happened in?

@MylesBorinsMylesBorins added this to the v4.6.2 milestone Oct 24, 2016
@mscdex
Copy link
Contributor

@thealphanerd#8948

@MylesBorinsMylesBorins modified the milestones: v4.7.0, v4.6.2Oct 26, 2016
@MylesBorins
Copy link
Contributor

@bnoordhuis or @mscdex would one of you be willing to backport this with the commit from #8948?

@jbergstroem
Copy link
Member

and on top, #9262.

mscdex pushed a commit to mscdex/io.js that referenced this pull request Nov 18, 2016
Teach gtest to produce TAP so we can integrate it better with our CI tooling. TAP is printed to stdout but it can also be written to file by passing the `--gtest_output=tap:filename.tap` switch to cctest. PR-URL: nodejs#8034 Reviewed-By: James M Snell <[email protected]>
mscdex pushed a commit to mscdex/io.js that referenced this pull request Nov 18, 2016
Enable the cctests on the CI now that they know how to write TAP output. PR-URL: nodejs#8034 Reviewed-By: James M Snell <[email protected]>
@mscdexmscdex mentioned this pull request Nov 18, 2016
2 tasks
@mscdex
Copy link
Contributor

@thealphanerd#9682

MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
Teach gtest to produce TAP so we can integrate it better with our CI tooling. TAP is printed to stdout but it can also be written to file by passing the `--gtest_output=tap:filename.tap` switch to cctest. PR-URL: #8034 Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
Enable the cctests on the CI now that they know how to write TAP output. PR-URL: #8034 Reviewed-By: James M Snell <[email protected]>
@MylesBorinsMylesBorins removed this from the 4.7.0 milestone Nov 22, 2016
@MylesBorinsMylesBorins mentioned this pull request Nov 22, 2016
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to build files or the CI.c++Issues and PRs that require attention from people who are familiar with C++.toolsIssues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@bnoordhuis@jbergstroem@joaocgreis@eugeneo@ofrobots@mscdex@MylesBorins@jasnell@richardlau@nodejs-github-bot