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

deps: check in gtest, add util unit test #1199

Merged
merged 1 commit into from
Apr 1, 2015
Merged

Conversation

bnoordhuis
Copy link
Member

Check in a gypified gtest and add a simple unit test to show that the
basic infrastructure is in place.

Refs: #1193

R=@indutny and /cc @jbergstroem

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/333/

@jbergstroem
Copy link
Member

I don't think we should build gtest unless invoking make cctest (or inherited through test, ..).

@indutny
Copy link
Member

indutny commented Mar 19, 2015

What @jbergstroem said, otherwise LGTM! Good job, @bnoordhuis

@jbergstroem
Copy link
Member

Blows up on SmartOS. Looking into it.

@bnoordhuis
Copy link
Member Author

Updated the Makefile to build out/Release/cctest only when running make cctest. It's a recursive make action (boo! slow!) but that can't be helped due to the way gyp works. PTAL.

@bnoordhuis
Copy link
Member Author

@jbergstroem
Copy link
Member

Come to think of it, this unfortunately won't show any build/test errors with gtest until it's landed and added to test-simple/message (for the foreseeable future). If others agree, I'd be ok with "abusing" Makefile for this branch until we're happy with the result, then clean before merging.

@bnoordhuis
Copy link
Member Author

I'd be ok with "abusing" Makefile for this branch until we're happy with the result, then clean before merging.

I'm not sure what you mean by that.

@jbergstroem
Copy link
Member

I'm not sure what you mean by that.

Abuse was a poor choice of word. What I meant was letting gtest build with plain make and possibly add it to test-simple so we can verify it on all platforms.

@jbergstroem
Copy link
Member

Re SmartOS: The problem is (was) that you built the tests twice and tried to link them; once from -all and then individually. This fixes it:

diff --git deps/gtest/gtest.gyp deps/gtest/gtest.gyp
index d58d065..90388e3 100644
--- deps/gtest/gtest.gyp
+++ deps/gtest/gtest.gyp
@@ -8,7 +8,6 @@
       },
       'include_dirs': ['.', 'include'],
       'sources': [
-        'src/gtest-all.cc',
         'src/gtest-death-test.cc',
         'src/gtest-filepath.cc',
         'src/gtest-internal-inl.h',

(also, feel free to kill that file)

I'd be keen to see some different versions of Windows have a run at this before we commit. Also, is it common gtest practise to have tests live with src? I'd prefer moving them to test/cctest (or similar) unless that's the case.

@jbergstroem
Copy link
Member

Also suggesting we split building tests from running them. I'd like to build everything necessary before running it -- makes output easier to follow (this needs to be fixed for other tests as well).

@jbergstroem
Copy link
Member

I might be running ahead of time here, but I'd suggest plugging this in from the get-go. Looks simple enough: https://github.com/kinow/gtest-tap-listener

Thoughts?

@mscdex mscdex added the test Issues and PRs related to the tests. label Mar 22, 2015
@jbergstroem
Copy link
Member

Did some minor work here: https://github.com/jbergstroem/io.js/tree/pr/1199

We need to provide TAP consumers with a bit more information about what suite is running.

@bnoordhuis
Copy link
Member Author

Incorporated @jbergstroem's fixups, PTAL.

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/414/

@jbergstroem Let me know if (and esp. how) you want attribution.

@jbergstroem
Copy link
Member

@bnoordhuis no attribution needed; just happy to see it go in.

@bnoordhuis
Copy link
Member Author

@jbergstroem pointed out that Jenkins currently runs make test-simple test-message instead of make test so here is a new CI with a hacked up test-simple target: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/415/

@bnoordhuis
Copy link
Member Author

Looks like it needs an update to vcvars.bat in order to build the test suite on Windows.

@bnoordhuis
Copy link
Member Author

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/422/

Side note: I think I'm going to revert the changes to the Makefile that make cctest a separate build target. Making it a dependency of the iojs target means no one can accidentally break the C++ test suite because they forgot to run make test first.

It's also consistent with the VS build (where cctest is always built) and it removes two recursive make calls from make test (making it a little faster.)

@jbergstroem
Copy link
Member

The test run looks ok. Good.

I don't think the argument of not breaking holds -- doesn't the same apply for all javascript functionality? My opinion is that cctest belongs with make test and once we've written a tool that parses the xml we run it from make test-ci so we get tap for all the things.

I'm not super happy about how our Makefile recurses All The Things, but I don't think we should "fix" that with this PR. How about possibly doing a build-test-step that calls builds cctest, addons, doctests, etc and then clean out all the other test build dependency targets?

Regardless if you insist on building cctest with the default target you have my LGTM on this. Great job; looking forward to simplifying the doc/addon test situation as well as expanding test coverage.

:shipit:

@bnoordhuis
Copy link
Member Author

The primary reason is that making it part of the main build keeps things consistent with Windows. I value consistency more than the few seconds it shaves off someone's non-test build.

I reordered things a little so that cctest runs before the JS tests. They didn't run on the ARM buildbots before because the JS test suite always fails due to timeouts. PTAL.

New (and hopefully final) CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/427/

@jbergstroem
Copy link
Member

Compilation order: Ok, it's a minor change but the purist in me will revisit this once the test suit grows :-)

The timeout stuff will go away when we migrate to test-ci. The tests are run in succession (as in: line by line in same build step) from Jenkins which means it bails if on non-zero return.

Changes and overall is LGTM.

Check in a gypified gtest and add a simple unit test to show that the
basic infrastructure is in place.

PR-URL: nodejs#1199
Refs: nodejs#1193
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants