Skip to content

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

Merged
MylesBorins merged 3 commits into
nodejs:v4.x-stagingfrom
MylesBorins:V8-testrunner-backport
Jun 29, 2016
Merged

[v4.x backport] test: run v8 tests from node tree#7451
MylesBorins merged 3 commits into
nodejs:v4.x-stagingfrom
MylesBorins:V8-testrunner-backport

Conversation

@MylesBorins

@MylesBorinsMylesBorins commented Jun 28, 2016

Copy link
Copy Markdown
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

@MylesBorinsMylesBorins 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

mscdex commented Jun 28, 2016

Copy link
Copy Markdown
Contributor

Are the log files necessary?

@mscdexmscdex added the test Issues and PRs related to the tests. label Jun 28, 2016
@targos

targos commented Jun 28, 2016

Copy link
Copy Markdown
Member

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

@targos

targos commented Jun 28, 2016

Copy link
Copy Markdown
Member

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

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

@bnoordhuis

bnoordhuis commented Jun 28, 2016

Copy link
Copy Markdown
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

bnoordhuis commented Jun 28, 2016

Copy link
Copy Markdown
Member

LGTM BTW

@targos

targos commented Jun 28, 2016

Copy link
Copy Markdown
Member

LGTM too

@MylesBorins

MylesBorins commented Jun 28, 2016•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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
MylesBorinsforce-pushed the V8-testrunner-backport branch 2 times, most recently from 8b991d8 to 5f92123CompareJune 28, 2016 08:27
@MylesBorins

MylesBorins commented Jun 28, 2016

Copy link
Copy Markdown
ContributorAuthor

@targos@bnoordhuis PTAL

targos
targos reviewed Jun 28, 2016
View reviewed changes
Comment threaddeps/v8/tools/run-tests.py Outdated

@targostargosJun 28, 2016

Copy link
Copy Markdown
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

@MylesBorinsMylesBorinsJun 28, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

fixed.

@MylesBorins
MylesBorinsforce-pushed the V8-testrunner-backport branch from 5f92123 to 6f6a2d7CompareJune 28, 2016 08:46
@bnoordhuis

bnoordhuis commented Jun 28, 2016

Copy link
Copy Markdown
Member

LGTM

1 similar comment
@targos

targos commented Jun 28, 2016

Copy link
Copy Markdown
Member

LGTM

@MylesBorinsMylesBorins changed the title test: run v8 tests from node treetest: run v8 tests from node tree [v4.x backport]Jun 28, 2016
@mhdawson

mhdawson commented Jun 28, 2016•
edited
Loading

Copy link
Copy Markdown
Member

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

mhdawson commented Jun 28, 2016•
edited
Loading

Copy link
Copy Markdown
Member

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

jasnell commented Jun 28, 2016

Copy link
Copy Markdown
Member

LGTM

@MylesBorins

MylesBorins commented Jun 28, 2016•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

MylesBorins commented Jun 28, 2016

Copy link
Copy Markdown
ContributorAuthor

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

@MylesBorinsMylesBorins 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
@MylesBorinsMylesBorins added the wip Issues and PRs that are still a work in progress. label Jun 29, 2016
@MylesBorinsMylesBorins changed the title [WIP] test: run v8 tests from node tree [v4.x backport][WIP][v4.x backport] test: run v8 tests from node treeJun 29, 2016
@exinfinitumMyles Borins
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
MylesBorinsforce-pushed the V8-testrunner-backport branch from 6f6a2d7 to 1496fecCompareJune 29, 2016 21:35
@MylesBorins

MylesBorins commented Jun 29, 2016•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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/

@MylesBorinsMylesBorins removed the wip Issues and PRs that are still a work in progress. label Jun 29, 2016
@MylesBorinsMylesBorins changed the title [WIP][v4.x backport] test: run v8 tests from node tree[v4.x backport] test: run v8 tests from node treeJun 29, 2016
MylesBorinsand 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>
Myles Borins@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
MylesBorinsforce-pushed the V8-testrunner-backport branch from 1496fec to 72de1ceCompareJune 29, 2016 23:03
@MylesBorins
MylesBorins merged commit 72de1ce into nodejs:v4.x-stagingJun 29, 2016
@MylesBorins

MylesBorins commented Jun 29, 2016

Copy link
Copy Markdown
ContributorAuthor

landed in 48949fb...72de1ce

@MylesBorinsMylesBorins mentioned this pull request Jun 29, 2016
Closed
@MylesBorins
MylesBorins deleted the V8-testrunner-backport branch July 8, 2016 14:51
@MylesBorinsMylesBorins mentioned this pull request Jul 12, 2016
Closed
MylesBorins added a commit that referenced this pull request Jul 12, 2016
@MylesBorinsMyles Borins
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
Myles Borins
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
@MylesBorinsMylesBorins 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: port9809992to 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

semver-minorPRs that contain new features and should be released in the next minor version.testIssues and PRs related to the tests.toolsIssues and PRs related to the tools directory.v8 engineIssues 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