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

[v4.x backport] test: run v8 tests from node tree #7451

Merged
merged 3 commits into from
Jun 29, 2016
Merged

[v4.x backport] test: run v8 tests from node tree #7451

merged 3 commits into from
Jun 29, 2016

Conversation

MylesBorins
Copy link
Contributor

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

tools

Description of change

This is a backport of cd720f8 from #4704 which enables running the V8 test suite on v4.x

ci: https://ci.nodejs.org/job/node-test-commit-v8-linux/157/

(there are currently 4 failures, not sure what they are related to)

/cc @nodejs/lts @nodejs/V8 @ofrobots @mhdawson

@MylesBorins MylesBorins added v8 engine Issues and PRs related to the V8 dependency. tools Issues and PRs related to the tools directory. v4.x semver-minor PRs that contain new features and should be released in the next minor version. labels Jun 28, 2016
@mscdex
Copy link
Contributor

mscdex commented Jun 28, 2016

Are the log files necessary?

@mscdex mscdex added the test Issues and PRs related to the tests. label Jun 28, 2016
@targos
Copy link
Member

targos commented Jun 28, 2016

The log files are necessary. They were filtered out by a rule in our .gitignore

@targos
Copy link
Member

targos commented Jun 28, 2016

Reference for the failures: https://bugs.chromium.org/p/v8/issues/detail?id=2899

See https://codereview.chromium.org/1402373002 for a solution.

@bnoordhuis
Copy link
Member

Hah, I was just about to post that v8/v8@9c927d0 should fix the test failures; the issue is with the default system locale.

@bnoordhuis
Copy link
Member

LGTM BTW

@targos
Copy link
Member

targos commented Jun 28, 2016

LGTM too

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Jun 28, 2016 •
edited
Loading

I've added a backport of v8/v8@9c927d0

V8 ci: https://ci.nodejs.org/job/node-test-commit-v8-linux/159/
ci: https://ci.nodejs.org/job/node-test-pull-request/3102/

@MylesBorins MylesBorins force-pushed the V8-testrunner-backport branch 2 times, most recently from 8b991d8 to 5f92123 Compare June 28, 2016 08:27
@MylesBorins
Copy link
Contributor Author

@targos @bnoordhuis PTAL

targos
targos reviewed Jun 28, 2016
View reviewed changes
deps/v8/tools/run-tests.py
@@ -326,6 +326,9 @@ def ProcessOptions(options):
global VARIANT_FLAGS
global VARIANTS

# Many tests assume an English interface.
os.environ['LANG'] = 'en_US.UTF-8'
Copy link
Member

Choose a reason for hiding this comment

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

the indentation is wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@bnoordhuis
Copy link
Member

LGTM

1 similar comment
@targos
Copy link
Member

targos commented Jun 28, 2016

LGTM

@MylesBorins MylesBorins changed the title test: run v8 tests from node tree test: run v8 tests from node tree [v4.x backport] Jun 28, 2016
@mhdawson
Copy link
Member

mhdawson commented Jun 28, 2016 •
edited
Loading

From what I can see its not running quite right in the CI. The output of the jobs look a bit different. What that on purpose (the full output of the tests being run versus the dots ?)

@mhdawson
Copy link
Member

mhdawson commented Jun 28, 2016 •
edited
Loading

Looking more closely I think the answer to my question is no, as the result is that the job shows as failed even though the tests appear to have passed. In particular the tap file with the results is not being generated:

+ cp deps/v8/v8-tap.xml v8-tap.xml
cp: cannot stat 'deps/v8/v8-tap.xml': No such file or directory

@jasnell
Copy link
Member

jasnell commented Jun 28, 2016

LGTM

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Jun 28, 2016 •
edited
Loading

One more run in CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/161/

@mhdawson I noticed that as well re: tap output. What is strange is that the first run of this change seemed to produce the tap output (this is where the failures came up). Can you think of any reason the tap output would not be produced?

@MylesBorins
Copy link
Contributor Author

I've opened #7460 which should solve the problem with the tap file. Once that has landed I'll add it to the backport

@MylesBorins MylesBorins changed the title test: run v8 tests from node tree [v4.x backport] [WIP] test: run v8 tests from node tree [v4.x backport] Jun 29, 2016
@MylesBorins MylesBorins added the wip Issues and PRs that are still a work in progress. label Jun 29, 2016
@MylesBorins MylesBorins changed the title [WIP] test: run v8 tests from node tree [v4.x backport] [WIP][v4.x backport] test: run v8 tests from node tree Jun 29, 2016
@exinfinitum
test: run v8 tests from node tree
5f9110e 
Ported by exinfinitum from a PR by jasnell:
see nodejs/node-v0.x-archive#14185

Allows the running of v8 tests on node's packaged v8 source code.

Note that the limited win32 support added by jasnell has NOT been ported,
and so these tests are currently UNIX ONLY.

Note that gclient depot tools
(see https://commondatastorage.googleapis.com/
chrome-infra-docs/flat/depot_tools/docs/html/
depot_tools_tutorial.html#_setting_up) and subversion are required
to run tests.

To perform tests, run the following commands:

make v8 DESTCPU=(ARCH)
make test-v8 DESTCPU=(ARCH)

where (ARCH) is your CPU architecture, e.g. x64, ia32.
DESTCPU MUST be specified for this to work properly.

Can also do tests on debug build by using "make test-v8 DESTCPU=(ARCH)
BUILDTYPE=Debug", or perform intl or benchmark tests via make
test-v8-intl or test-v8-benchmarks respectively.

Note that by default, quickcheck and TAP output are disabled, and i18n
is enabled. To activate these options, use options"QUICKCHECK=True" and
"ENABLE_V8_TAP=True" respectively.

Use "DISABLE_V8_I18N" to disable i18n.

Use V8_BUILD_OPTIONS to allow custom user-defined flags to be
appended onto "make v8".

Any tests performed after changes to the packaged v8 file will require
recompiling of v8, which can be done using "make v8 DESTCPU=(ARCH)".

Finally, two additional files necessary for one of the v8 tests have
been added to the v8 folder.

PR-URL: nodejs#4704
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
@MylesBorins
Copy link
Contributor Author

MylesBorins commented Jun 29, 2016 •
edited
Loading

I've now included a backport of the fix from upstream... running CI one more time. Will land in v4.x-staging in green.

ci: https://ci.nodejs.org/job/node-test-commit-v8-linux/167/

@MylesBorins MylesBorins removed the wip Issues and PRs that are still a work in progress. label Jun 29, 2016
@MylesBorins MylesBorins changed the title [WIP][v4.x backport] test: run v8 tests from node tree [v4.x backport] test: run v8 tests from node tree Jun 29, 2016
MylesBorins and others added 2 commits June 29, 2016 16:03
@MylesBorins
deps: backport 9c927d0f01 from V8 upstream
c6fab00 
Original commit message:

    [test] Set default locale in test runner

    BUG=v8:4437,v8:2899,chromium:604310
    LOG=n

    Review URL: https://codereview.chromium.org/1402373002

    Cr-Commit-Position: refs/heads/master@{nodejs#35614}

PR-URL: nodejs#7451
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
tools: explicit path for V8 test tap output
72de1ce 
Currently we do not specific an absolute path for the tap output of the
V8 test suite. This is proving to be unreliable across release lines.

By prepending `$(PWD)` to each path we can guarantee it will always be
in the root folder.

PR-URL: nodejs#7460
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorins MylesBorins merged commit 72de1ce into nodejs:v4.x-staging Jun 29, 2016
@MylesBorins
Copy link
Contributor Author

landed in 48949fb...72de1ce

@MylesBorins MylesBorins mentioned this pull request Jun 29, 2016
Closed
@MylesBorins MylesBorins deleted the V8-testrunner-backport branch July 8, 2016 14:51
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
Closed
MylesBorins added a commit that referenced this pull request Jul 12, 2016
@MylesBorins
deps: backport 9c927d0f01 from V8 upstream
9809992 
Original commit message:

    [test] Set default locale in test runner

    BUG=v8:4437,v8:2899,chromium:604310
    LOG=n

    Review URL: https://codereview.chromium.org/1402373002

    Cr-Commit-Position: refs/heads/master@{#35614}

PR-URL: #7451
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
2016-07-12, Version 4.5.0 'Argon' (LTS)
3103a79 
Notable Changes:

This list is not yet complete. Please comment in the thread with
commits you think should be included. Descriptions will also be
updated in a later release candidate.

Semver Minor:

* buffer:
 * backport new buffer constructor APIs to v4.x
   (Сковорода Никита Андреевич)
   #7562
 * ignore negative allocation lengths (Anna Henningsen)
   #7562
 * backport --zero-fill-buffers cli option (James M Snell)
   #5745
* build:
  * add Intel Vtune profiling support (Chunyang Dai)
    #5527
* repl:
  * copying tabs shouldn't trigger completion (Eugene Obrezkov)
    #5958
* src:
  * add node::FreeEnvironment public API (Cheng Zhao)
    #3098
* test:
  * run v8 tests from node tree (Bryon Leung)
    #4704
* V8:
  * backport 9c927d0f01 from V8 upstream (Myles Borins)
    #7451
  * cherry-pick 68e89fb from v8's upstream (Fedor Indutny)
    #3779

Semver Patch:

* **libuv**:
  * upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    #6796
  * upgrade libuv to 1.9.0 (Saúl Ibarra Corretgé)
    #5994
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
Merged
richardlau added a commit to ibmruntimes/node that referenced this pull request Jul 14, 2016
@richardlau
deps: port 9809992 to deps/v8z
860020b 
Original commit message:

deps: backport 9c927d0f01 from V8 upstream

Original commit message:

    [test] Set default locale in test runner

    BUG=v8:4437,v8:2899,chromium:604310
    LOG=n

    Review URL: https://codereview.chromium.org/1402373002

    Cr-Commit-Position: refs/heads/master@{#35614}

PR-URL: nodejs/node#7451
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
semver-minor PRs that contain new features and should be released in the next minor version. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@MylesBorins @mscdex @targos @bnoordhuis @mhdawson @jasnell @exinfinitum