Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

test: integrate v8 test suite #9208

Closed
wants to merge 1 commit into from
Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Feb 13, 2015

Make it possible to run the basic v8 test suite.

To get started, include --with-v8-tests on configure. Running
make will then build the test suite.

./configure --with-v8-tests
make
make test-v8

If you include i18n when you build, you can run make test-v8-intl

./configure --with-v8-tests --with-intl=full-icu
make
make test-v8-intl

Quick checks only. No presubmits. Release mode.

The commit includes some patches to the v8 dependency. It seems
that the .gitignore filters out some necessary log files and
intl/collator/default-locale appears to be working on macos 10.10.

/cc @misterdjules @tjfontaine - Please take a look and let me know if
this is a good approach..

Make it possible to run the basic v8 test suite.

To get started, include `--with-v8-tests` on `configure`. Running
`make` will then
build the test suite.

```bash
./configure --with-v8-tests
make
make test-v8
```
If you include i18n when you build, you can run `make test-v8-int`
```bash
./configure --with-v8-tests --with-intl=full-icu
make
make test-v8-intl
```

Quick checks only. No presubmits. Release mode.

The commit includes some patches to the v8 dependency. It seems that
the .gitignore filters out some necessary log files and
collator/default-locale
appears to be working on macos 10.10.
@jasnell
Copy link
Member Author

jasnell commented Feb 13, 2015

Oh... one very important additional point: one of the tests (cctest) requires C++11 in order to compile. To build successfully on mac I had to set std=c++11. Obviously this is a departure from the current build...

export CXX="`which clang++` -std=c++11"
export LINK="`which clang++` -std=c++11"
export GYP_DEFINES="clang=1"

@trevnorris
Copy link

Nice work.

/cc @tjfontaine @cjihrig

@cjihrig
Copy link

cjihrig commented Feb 13, 2015

+1

@trevnorris
Copy link

@misterdjules Know if this would change anything with Jenkins?

misterdjules pushed a commit to misterdjules/node that referenced this pull request Feb 16, 2015
431eb17 had integrated the addition of
V8's version in V8's profiler log files, without backporting the test
that was included in the original change
(https://codereview.chromium.org/806143002). This commit backports this
test.

The newly added test was tested with
nodejs#9208.
misterdjules pushed a commit to misterdjules/node that referenced this pull request Feb 16, 2015
431eb17 had integrated the addition of
V8's version in V8's profiler log files, without backporting the test
that was included in the original change
(https://codereview.chromium.org/806143002). This commit backports this
test.

The newly added test was tested with
nodejs#9208.
misterdjules pushed a commit to misterdjules/node that referenced this pull request Feb 17, 2015
431eb17 had integrated the addition of
V8's version in V8's profiler log files, without backporting the test
that was included in the original change
(https://codereview.chromium.org/806143002). This commit backports this
test.

The newly added test was tested with
nodejs#9208.
@jasnell
Copy link
Member Author

jasnell commented Mar 5, 2015

cc @joyent/node-coreteam ... where are we at with this?

@trevnorris
Copy link

/cc @misterdjules

@misterdjules
Copy link

@jasnell @trevnorris Please accept my apologies for the delay in my response.

First, thank you very much James for this!

I tried to run ./configure --with-v8-tests && make and I got the following output:

LD_LIBRARY_PATH=/Users/JulienGilli/dev/node/v0.12/out/Release/lib.host:/Users/JulienGilli/dev/node/v0.12/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../.; mkdir -p deps/v8/testing; svn checkout --force https://googletest.googlecode.com/svn/trunk/ deps/v8/testing/gtest --revision 692
Error validating server certificate for 'https://googletest.googlecode.com:443':
 - The certificate is not issued by a trusted authority. Use the
   fingerprint to validate the certificate manually!
Certificate information:
 - Hostname: *.googlecode.com
 - Valid: from Fri, 27 Feb 2015 20:12:30 GMT until Thu, 28 May 2015 00:00:00 GMT
 - Issuer: Google Inc, US
 - Fingerprint: 9c:df:52:b3:2a:2c:11:16:a1:c2:ba:78:66:ba:f6:84:31:0e:72:5d
(R)eject, accept (t)emporarily or accept (p)ermanently? t
svn: E175002: Server sent unexpected return value (502 Bad Gateway) in response to PROPFIND request for '/svn/trunk'
make[1]: *** [deps/v8/testing/gtest] Error 1
make: *** [node] Error 2

I had tested it before (around the time the PR was submitted), and it had worked well, so I'm not sure if I'm doing something wrong or if something changed on google's side in the meantime. Did you try to build it recently?

Other than that, in my opinion it would be better to download gtest only if we want to run V8's tests. In other words, building the default target would not do anything different than what it does currently, and gtest would be donwloaded only when building test-v8* targets.

I would also put the additional GYP configuration in a separate file and keep only what is needed to build the node binary in node.gyp.

As for the requirement on C++11, do we know exactly what code needs what feature(s) of C++11?

Thank you very much again!

@jasnell
Copy link
Member Author

jasnell commented Mar 7, 2015

I can go back and tweak the build so that the test stuff is built only if
the test-v8 options are selected. Give me about a week on that.

There is one test class that fails without the C++11 stuff. It may be
possible to exclude it.
On Mar 6, 2015 5:30 PM, "Julien Gilli" [email protected] wrote:

@jasnell https://github.com/jasnell @trevnorris
https://github.com/trevnorris Please accept my apologies for the delay
in my response.

First, thank you very much James for this!

I tried to run ./configure --with-v8-tests && make and I got the
following output:

LD_LIBRARY_PATH=/Users/JulienGilli/dev/node/v0.12/out/Release/lib.host:/Users/JulienGilli/dev/node/v0.12/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../.; mkdir -p deps/v8/testing; svn checkout --force https://googletest.googlecode.com/svn/trunk/ deps/v8/testing/gtest --revision 692
Error validating server certificate for 'https://googletest.googlecode.com:443':

  • The certificate is not issued by a trusted authority. Use the
    fingerprint to validate the certificate manually!
    Certificate information:
  • Hostname: *.googlecode.com
  • Valid: from Fri, 27 Feb 2015 20:12:30 GMT until Thu, 28 May 2015 00:00:00 GMT
  • Issuer: Google Inc, US
  • Fingerprint: 9c:df:52:b3:2a:2c:11:16:a1:c2:ba:78:66:ba:f6:84:31:0e:72:5d
    (R)eject, accept (t)emporarily or accept (p)ermanently? t
    svn: E175002: Server sent unexpected return value (502 Bad Gateway) in response to PROPFIND request for '/svn/trunk'
    make[1]: *** [deps/v8/testing/gtest] Error 1
    make: *** [node] Error 2

I had tested it before (around the time the PR was submitted), and it had
worked well, so I'm not sure if I'm doing something wrong or if something
changed on google's side in the meantime. Did you try to build it recently?

Other than that, in my opinion it would be better to download gtest only
if we want to run V8's tests. In other words, building the default target
would not do anything different than what it does currently, and gtest
would be donwloaded only when building test-v8* targets.

I would also put the additional GYP configuration in a separate file and
keep only what is needed to build the node binary in node.gyp.

As for the requirement on C++11, do we know exactly what code needs what
feature(s) of C++11?

Thank you very much again!


Reply to this email directly or view it on GitHub
#9208 (comment).

@misterdjules
Copy link

Sounds good, thank you @jasnell!

@piscisaureus
Copy link

Other than that, in my opinion it would be better to download gtest only if we want to run V8's tests. In other words, building the default target would not do anything different than what it does currently, and gtest would be donwloaded only when building test-v8* targets.

I would recommend against downloading stuff as part of a build step. I suggest just putting the tests in git.

@jasnell jasnell mentioned this pull request Mar 28, 2015
@jasnell
Copy link
Member Author

jasnell commented Mar 28, 2015

Replaced with #14185

@jasnell jasnell closed this Mar 28, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants