FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: fix argument computation in embedtest · nodejs/node@1fffda5 · GitHub

/ node Public

Commit 1fffda5

Browse files
joyeecheung
authored and
ruyadorno
committed
test: fix argument computation in embedtest
There were a few bugs in the original test that went unnoticed because with the bug the test did not actually get run anymore. This patch fixes the argument computation by accounting filtering of the arguments, and uses spawnSyncAndExit{WithoutError} in the test to enable better logging when the test fails. PR-URL: #49506 Fixes: #49501 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 6010a91 commit 1fffda5

2 files changed

Lines changed: 116 additions & 66 deletions

File tree

‎test/embedding/embedtest.cc‎

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ int RunNodeInstance(MultiIsolatePlatform* platform,
8989
snapshot_as_file = true;
9090
} else if (arg == "--embedder-snapshot-blob") {
9191
assert(i + 1 < args.size());
92-
snapshot_blob_path = args[i + i];
92+
snapshot_blob_path = args[i + 1];
9393
i++;
9494
} else {
9595
filtered_args.push_back(arg);
@@ -121,9 +121,10 @@ int RunNodeInstance(MultiIsolatePlatform* platform,
121121

122122
if (is_building_snapshot) {
123123
// It contains at least the binary path, the code to snapshot,
124-
// and --embedder-snapshot-create. Insert an anonymous filename
125-
// as process.argv[1].
126-
assert(filtered_args.size() >= 3);
124+
// and --embedder-snapshot-create (which is filtered, so at least
125+
// 2 arguments should remain after filtering).
126+
assert(filtered_args.size() >= 2);
127+
// Insert an anonymous filename as process.argv[1].
127128
filtered_args.insert(filtered_args.begin() + 1,
128129
node::GetAnonymousMainPath());
129130
}
@@ -153,19 +154,26 @@ int RunNodeInstance(MultiIsolatePlatform* platform,
153154
Context::Scope context_scope(setup->context());
154155

155156
MaybeLocal<Value> loadenv_ret;
156-
if (snapshot) {
157+
if (snapshot) { // Deserializing snapshot
157158
loadenv_ret = node::LoadEnvironment(env, node::StartExecutionCallback{});
158-
} else {
159+
} else if (is_building_snapshot) {
160+
// Environment created for snapshotting must set process.argv[1] to
161+
// the name of the main script, which was inserted above.
159162
loadenv_ret = node::LoadEnvironment(
160163
env,
161-
// Snapshots do not support userland require()s (yet)
162-
"if (!require('v8').startupSnapshot.isBuildingSnapshot()) {"
163-
" const publicRequire ="
164-
" require('module').createRequire(process.cwd() + '/');"
165-
" globalThis.require = publicRequire;"
166-
"} else globalThis.require = require;"
164+
"const assert = require('assert');"
165+
"assert(require('v8').startupSnapshot.isBuildingSnapshot());"
167166
"globalThis.embedVars = { nön_ascıı: '🏳️‍🌈' };"
167+
"globalThis.require = require;"
168168
"require('vm').runInThisContext(process.argv[2]);");
169+
} else {
170+
loadenv_ret = node::LoadEnvironment(
171+
env,
172+
"const publicRequire = require('module').createRequire(process.cwd() "
173+
"+ '/');"
174+
"globalThis.require = publicRequire;"
175+
"globalThis.embedVars = { nön_ascıı: '🏳️‍🌈' };"
176+
"require('vm').runInThisContext(process.argv[1]);");
169177
}
170178

171179
if (loadenv_ret.IsEmpty()) // There has been a JS exception.

‎test/embedding/test-embedding.js‎

Lines changed: 96 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,10 @@ const common = require('../common');
33
const fixtures = require('../common/fixtures');
44
const tmpdir = require('../common/tmpdir');
55
const assert = require('assert');
6-
const child_process = require('child_process');
6+
const {
7+
spawnSyncAndExit,
8+
spawnSyncAndExitWithoutError,
9+
} = require('../common/child_process');
710
const path = require('path');
811
const fs = require('fs');
912

@@ -21,39 +24,54 @@ function resolveBuiltBinary(bin) {
2124

2225
const binary = resolveBuiltBinary('embedtest');
2326

24-
assert.strictEqual(
25-
child_process.spawnSync(binary, ['console.log(42)'])
26-
.stdout.toString().trim(),
27-
'42');
28-
29-
assert.strictEqual(
30-
child_process.spawnSync(binary, ['console.log(embedVars.nön_ascıı)'])
31-
.stdout.toString().trim(),
32-
'🏳️‍🌈');
33-
34-
assert.strictEqual(
35-
child_process.spawnSync(binary, ['console.log(42)'])
36-
.stdout.toString().trim(),
37-
'42');
27+
spawnSyncAndExitWithoutError(
28+
binary,
29+
['console.log(42)'],
30+
{
31+
trim: true,
32+
stdout: '42',
33+
});
3834

39-
assert.strictEqual(
40-
child_process.spawnSync(binary, ['throw new Error()']).status,
41-
1);
35+
spawnSyncAndExitWithoutError(
36+
binary,
37+
['console.log(embedVars.nön_ascıı)'],
38+
{
39+
trim: true,
40+
stdout: '🏳️‍🌈',
41+
});
4242

43-
// Cannot require internals anymore:
44-
assert.strictEqual(
45-
child_process.spawnSync(binary, ['require("lib/internal/test/binding")']).status,
46-
1);
43+
spawnSyncAndExit(
44+
binary,
45+
['throw new Error()'],
46+
{
47+
status: 1,
48+
signal: null,
49+
});
4750

48-
assert.strictEqual(
49-
child_process.spawnSync(binary, ['process.exitCode = 8']).status,
50-
8);
51+
spawnSyncAndExit(
52+
binary,
53+
['require("lib/internal/test/binding")'],
54+
{
55+
status: 1,
56+
signal: null,
57+
});
5158

59+
spawnSyncAndExit(
60+
binary,
61+
['process.exitCode = 8'],
62+
{
63+
status: 8,
64+
signal: null,
65+
});
5266

5367
const fixturePath = JSON.stringify(fixtures.path('exit.js'));
54-
assert.strictEqual(
55-
child_process.spawnSync(binary, [`require(${fixturePath})`, 92]).status,
56-
92);
68+
spawnSyncAndExit(
69+
binary,
70+
[`require(${fixturePath})`, 92],
71+
{
72+
status: 92,
73+
signal: null,
74+
});
5775

5876
function getReadFileCodeForPath(path) {
5977
return `(require("fs").readFileSync(${JSON.stringify(path)}, "utf8"))`;
@@ -64,31 +82,49 @@ for (const extraSnapshotArgs of [[], ['--embedder-snapshot-as-file']]) {
6482
// readSync + eval since snapshots don't support userland require() (yet)
6583
const snapshotFixture = fixtures.path('snapshot', 'echo-args.js');
6684
const blobPath = tmpdir.resolve('embedder-snapshot.blob');
67-
const buildSnapshotArgs = [
85+
const buildSnapshotExecArgs = [
6886
`eval(${getReadFileCodeForPath(snapshotFixture)})`, 'arg1', 'arg2',
87+
];
88+
const embedTestBuildArgs = [
6989
'--embedder-snapshot-blob', blobPath, '--embedder-snapshot-create',
7090
...extraSnapshotArgs,
7191
];
72-
const runEmbeddedArgs = [
73-
'--embedder-snapshot-blob', blobPath, ...extraSnapshotArgs, 'arg3', 'arg4',
92+
const buildSnapshotArgs = [
93+
...buildSnapshotExecArgs,
94+
...embedTestBuildArgs,
95+
];
96+
97+
const runSnapshotExecArgs = [
98+
'arg3', 'arg4',
99+
];
100+
const embedTestRunArgs = [
101+
'--embedder-snapshot-blob', blobPath,
102+
...extraSnapshotArgs,
103+
];
104+
const runSnapshotArgs = [
105+
...runSnapshotExecArgs,
106+
...embedTestRunArgs,
74107
];
75108

76109
fs.rmSync(blobPath, { force: true });
77-
const child = child_process.spawnSync(binary, [
78-
'--', ...buildSnapshotArgs,
79-
], {
80-
cwd: tmpdir.path,
81-
});
82-
if (child.status !== 0) {
83-
console.log(child.stderr.toString());
84-
console.log(child.stdout.toString());
85-
}
86-
assert.strictEqual(child.status, 0);
87-
const spawnResult = child_process.spawnSync(binary, ['--', ...runEmbeddedArgs]);
88-
assert.deepStrictEqual(JSON.parse(spawnResult.stdout), {
89-
originalArgv: [binary, ...buildSnapshotArgs],
90-
currentArgv: [binary, ...runEmbeddedArgs],
91-
});
110+
spawnSyncAndExitWithoutError(
111+
binary,
112+
[ '--', ...buildSnapshotArgs ],
113+
{ cwd: tmpdir.path },
114+
{});
115+
spawnSyncAndExitWithoutError(
116+
binary,
117+
[ '--', ...runSnapshotArgs ],
118+
{ cwd: tmpdir.path },
119+
{
120+
stdout(output) {
121+
assert.deepStrictEqual(JSON.parse(output), {
122+
originalArgv: [binary, '__node_anonymous_main', ...buildSnapshotExecArgs],
123+
currentArgv: [binary, ...runSnapshotExecArgs],
124+
});
125+
return true;
126+
},
127+
});
92128
}
93129

94130
// Create workers and vm contexts after deserialization
@@ -99,14 +135,20 @@ for (const extraSnapshotArgs of [[], ['--embedder-snapshot-as-file']]) {
99135
`eval(${getReadFileCodeForPath(snapshotFixture)})`,
100136
'--embedder-snapshot-blob', blobPath, '--embedder-snapshot-create',
101137
];
138+
const runEmbeddedArgs = [
139+
'--embedder-snapshot-blob', blobPath,
140+
];
102141

103142
fs.rmSync(blobPath, { force: true });
104-
assert.strictEqual(child_process.spawnSync(binary, [
105-
'--', ...buildSnapshotArgs,
106-
], {
107-
cwd: tmpdir.path,
108-
}).status, 0);
109-
assert.strictEqual(
110-
child_process.spawnSync(binary, ['--', '--embedder-snapshot-blob', blobPath]).status,
111-
0);
143+
144+
spawnSyncAndExitWithoutError(
145+
binary,
146+
[ '--', ...buildSnapshotArgs ],
147+
{ cwd: tmpdir.path },
148+
{});
149+
spawnSyncAndExitWithoutError(
150+
binary,
151+
[ '--', ...runEmbeddedArgs ],
152+
{ cwd: tmpdir.path },
153+
{});
112154
}

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL