test: spawn new processes in vm-cached-data by indutny · Pull Request #6280 · nodejs/node · GitHub

Checklist

tests and code linting passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

test

Description of change

V8 may start caching scripts from the first run soon. Preventively
execute scripts in another process to ensure no test failures due to
an update in the future.

See: #6258

V8 may start caching scripts from the first run soon. Preventively execute scripts in another process to ensure no test failures due to an update in the future. See: nodejs#6258

cc @jeisinger

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

cool, LGTM

The return value isn't used. I think you intended to console.log or process.stdout.write it?

It is JS, the last statement will be printed when using -p argv.

Hm, very subtle. I guess it's not wrong but it's not very obviously correct, either. I'd use -e and explicitly print it.

Ack, I agree.

LGTM but with a strong suggestion to change the test.

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

Buffer.from(out.stdout.toString(), 'base64')

Ack.

LGTM with a nit if CI is green

Just for my understanding, process.argv[1] === undefined here, right?

It is not, see source on line 32.

$ node -e 'console.log(process.argv[1])' hello
hello

Bet you didn't know about this 😉

Oh, I missed the third argument. Never mind.

Landed in e1cf634, thank you everyone!

V8 may start caching scripts from the first run soon. Preventively execute scripts in another process to ensure no test failures due to an update in the future. See: #6258 PR-URL: #6280 Reviewed-By: Jochen Eisinger <jochen@chromium.org> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Marking it don't land in v4 simply because the future update to v8 does not apply

marking dont-land-on-v5.x for the same reason.

edit: if we want to backport we can use the commit above. I'll likely delete that branch in the next week if we do not opt for landing it

V8 may cache compiled scripts, and it is not safe to assume that the V8 flags can be changed after an isolate has been created. In this particular case, since we are using the same script multiple times, the test would fail if V8 cached the result of the compilation. Ref: nodejs#6280 Fixes: nodejs#6258

V8 may cache compiled scripts, and it is not safe to assume that the V8 flags can be changed after an isolate has been created. In this particular case, since we are using the same script multiple times, the test would fail if V8 cached the result of the compilation. Ref: #6280 Fixes: #6258 PR-URL: #6316 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com> Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>

V8 may start caching scripts from the first run soon. Preventively execute scripts in another process to ensure no test failures due to an update in the future. See: nodejs#6258 PR-URL: nodejs#6280 Reviewed-By: Jochen Eisinger <jochen@chromium.org> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

V8 may cache compiled scripts, and it is not safe to assume that the V8 flags can be changed after an isolate has been created. In this particular case, since we are using the same script multiple times, the test would fail if V8 cached the result of the compilation. Ref: nodejs#6280 Fixes: nodejs#6258 PR-URL: nodejs#6316 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com> Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>

V8 may start caching scripts from the first run soon. Preventively execute scripts in another process to ensure no test failures due to an update in the future. See: #6258 PR-URL: #6280 Reviewed-By: Jochen Eisinger <jochen@chromium.org> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

V8 may cache compiled scripts, and it is not safe to assume that the V8 flags can be changed after an isolate has been created. In this particular case, since we are using the same script multiple times, the test would fail if V8 cached the result of the compilation. Ref: #6280 Fixes: #6258 PR-URL: #6316 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com> Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>

While `let` no longer needs to run in `strict mode` in v8 5.x it throws in v8 4. This modification will make the test-vm-cached-data work in older version of node. Refs: nodejs#6280 PR-URL: nodejs#6317 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

While `let` no longer needs to run in `strict mode` in v8 5.x it throws in v8 4. This modification will make the test-vm-cached-data work in older version of node. Refs: #6280 PR-URL: #6317 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

test: spawn new processes in vm-cached-data

7c07bd8

V8 may start caching scripts from the first run soon. Preventively execute scripts in another process to ensure no test failures due to an update in the future. See: nodejs#6258

indutny force-pushed the fix/use-spawn-sync-in-vm-cached-data branch from 57da6a7 to 7c07bd8 Compare April 19, 2016 15:44

mscdex added vm Issues and PRs related to the vm subsystem. test Issues and PRs related to the tests. labels Apr 19, 2016

bnoordhuis reviewed Apr 20, 2016
View reviewed changes

fix

85c7fb7

jasnell reviewed Apr 20, 2016
View reviewed changes

...

56b1001

bnoordhuis reviewed Apr 20, 2016
View reviewed changes

indutny closed this Apr 20, 2016

indutny deleted the fix/use-spawn-sync-in-vm-cached-data branch April 20, 2016 16:35

jasnell added the dont-land-on-v4.x label Apr 20, 2016

MylesBorins added the dont-land-on-v5.x label Apr 20, 2016

ofrobots mentioned this pull request Apr 20, 2016

test: v8-flags is sensitive to script caching #6316

Closed

2 tasks

MylesBorins mentioned this pull request Apr 20, 2016

test: fix test-vm-cached-data to work with old v8 #6317

Merged

+                    assert(!script.cachedDataProduced || script.cachedData instanceof Buffer);
+                    if (script.cachedDataProduced)
+                      script.cachedData.toString('base64');

+                assert.equal(out.status, 0, out.stderr + '');
-                return script.cachedData;
+                return new Buffer(out.stdout.toString(), 'base64');

+                  let data;
+                  for (var i = 0; i < ${count}; i++) {
+                    var script = new vm.Script(process.argv[1], {

test: spawn new processes in vm-cached-data by indutny · Pull Request #6280 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: spawn new processes in vm-cached-data - #6280

test: spawn new processes in vm-cached-data#6280indutny wants to merge 3 commits intonodejs:masternodejs/node:masterfrom indutny:fix/use-spawn-sync-in-vm-cached-dataindutny/io.js:fix/use-spawn-sync-in-vm-cached-dataCopy head branch name to clipboard

Conversation

indutny commented Apr 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

indutny commented Apr 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

indutny commented Apr 19, 2016

Uh oh!

jeisinger commented Apr 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Apr 20, 2016

Uh oh!

indutny commented Apr 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

indutny commented Apr 20, 2016

Uh oh!

jasnell commented Apr 20, 2016

Uh oh!

MylesBorins commented Apr 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

test: spawn new processes in vm-cached-data#6280
indutny wants to merge 3 commits into
nodejs:masterfrom
indutny:fix/use-spawn-sync-in-vm-cached-data

indutny commented Apr 19, 2016 •
edited
Loading

indutny commented Apr 19, 2016 •
edited
Loading

MylesBorins commented Apr 20, 2016 •
edited
Loading