Skip to content

Commit

Permalink
test: fix argument computation in embedtest
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
  • Loading branch information
@joyeecheung @ruyadorno
joyeecheung authored and ruyadorno committed Sep 28, 2023
1 parent 6010a91 commit 1fffda5
Show file tree
Hide file tree
Showing 2 changed files with 116 additions and 66 deletions.
32 changes: 20 additions & 12 deletions test/embedding/embedtest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ int RunNodeInstance(MultiIsolatePlatform* platform,
snapshot_as_file = true;
} else if (arg == "--embedder-snapshot-blob") {
assert(i + 1 < args.size());
snapshot_blob_path = args[i + i];
snapshot_blob_path = args[i + 1];
i++;
} else {
filtered_args.push_back(arg);
Expand Down Expand Up @@ -121,9 +121,10 @@ int RunNodeInstance(MultiIsolatePlatform* platform,

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

MaybeLocal<Value> loadenv_ret;
if (snapshot) {
if (snapshot) { // Deserializing snapshot
loadenv_ret = node::LoadEnvironment(env, node::StartExecutionCallback{});
} else {
} else if (is_building_snapshot) {
// Environment created for snapshotting must set process.argv[1] to
// the name of the main script, which was inserted above.
loadenv_ret = node::LoadEnvironment(
env,
// Snapshots do not support userland require()s (yet)
"if (!require('v8').startupSnapshot.isBuildingSnapshot()) {"
" const publicRequire ="
" require('module').createRequire(process.cwd() + '/');"
" globalThis.require = publicRequire;"
"} else globalThis.require = require;"
"const assert = require('assert');"
"assert(require('v8').startupSnapshot.isBuildingSnapshot());"
"globalThis.embedVars = { nön_ascıı: '🏳️‍🌈' };"
"globalThis.require = require;"
"require('vm').runInThisContext(process.argv[2]);");
} else {
loadenv_ret = node::LoadEnvironment(
env,
"const publicRequire = require('module').createRequire(process.cwd() "
"+ '/');"
"globalThis.require = publicRequire;"
"globalThis.embedVars = { nön_ascıı: '🏳️‍🌈' };"
"require('vm').runInThisContext(process.argv[1]);");
}

if (loadenv_ret.IsEmpty()) // There has been a JS exception.
Expand Down
150 changes: 96 additions & 54 deletions test/embedding/test-embedding.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ const common = require('../common');
const fixtures = require('../common/fixtures');
const tmpdir = require('../common/tmpdir');
const assert = require('assert');
const child_process = require('child_process');
const {
spawnSyncAndExit,
spawnSyncAndExitWithoutError,
} = require('../common/child_process');
const path = require('path');
const fs = require('fs');

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

const binary = resolveBuiltBinary('embedtest');

assert.strictEqual(
child_process.spawnSync(binary, ['console.log(42)'])
.stdout.toString().trim(),
'42');

assert.strictEqual(
child_process.spawnSync(binary, ['console.log(embedVars.nön_ascıı)'])
.stdout.toString().trim(),
'🏳️‍🌈');

assert.strictEqual(
child_process.spawnSync(binary, ['console.log(42)'])
.stdout.toString().trim(),
'42');
spawnSyncAndExitWithoutError(
binary,
['console.log(42)'],
{
trim: true,
stdout: '42',
});

assert.strictEqual(
child_process.spawnSync(binary, ['throw new Error()']).status,
1);
spawnSyncAndExitWithoutError(
binary,
['console.log(embedVars.nön_ascıı)'],
{
trim: true,
stdout: '🏳️‍🌈',
});

// Cannot require internals anymore:
assert.strictEqual(
child_process.spawnSync(binary, ['require("lib/internal/test/binding")']).status,
1);
spawnSyncAndExit(
binary,
['throw new Error()'],
{
status: 1,
signal: null,
});

assert.strictEqual(
child_process.spawnSync(binary, ['process.exitCode = 8']).status,
8);
spawnSyncAndExit(
binary,
['require("lib/internal/test/binding")'],
{
status: 1,
signal: null,
});

spawnSyncAndExit(
binary,
['process.exitCode = 8'],
{
status: 8,
signal: null,
});

const fixturePath = JSON.stringify(fixtures.path('exit.js'));
assert.strictEqual(
child_process.spawnSync(binary, [`require(${fixturePath})`, 92]).status,
92);
spawnSyncAndExit(
binary,
[`require(${fixturePath})`, 92],
{
status: 92,
signal: null,
});

function getReadFileCodeForPath(path) {
return `(require("fs").readFileSync(${JSON.stringify(path)}, "utf8"))`;
Expand All @@ -64,31 +82,49 @@ for (const extraSnapshotArgs of [[], ['--embedder-snapshot-as-file']]) {
// readSync + eval since snapshots don't support userland require() (yet)
const snapshotFixture = fixtures.path('snapshot', 'echo-args.js');
const blobPath = tmpdir.resolve('embedder-snapshot.blob');
const buildSnapshotArgs = [
const buildSnapshotExecArgs = [
`eval(${getReadFileCodeForPath(snapshotFixture)})`, 'arg1', 'arg2',
];
const embedTestBuildArgs = [
'--embedder-snapshot-blob', blobPath, '--embedder-snapshot-create',
...extraSnapshotArgs,
];
const runEmbeddedArgs = [
'--embedder-snapshot-blob', blobPath, ...extraSnapshotArgs, 'arg3', 'arg4',
const buildSnapshotArgs = [
...buildSnapshotExecArgs,
...embedTestBuildArgs,
];

const runSnapshotExecArgs = [
'arg3', 'arg4',
];
const embedTestRunArgs = [
'--embedder-snapshot-blob', blobPath,
...extraSnapshotArgs,
];
const runSnapshotArgs = [
...runSnapshotExecArgs,
...embedTestRunArgs,
];

fs.rmSync(blobPath, { force: true });
const child = child_process.spawnSync(binary, [
'--', ...buildSnapshotArgs,
], {
cwd: tmpdir.path,
});
if (child.status !== 0) {
console.log(child.stderr.toString());
console.log(child.stdout.toString());
}
assert.strictEqual(child.status, 0);
const spawnResult = child_process.spawnSync(binary, ['--', ...runEmbeddedArgs]);
assert.deepStrictEqual(JSON.parse(spawnResult.stdout), {
originalArgv: [binary, ...buildSnapshotArgs],
currentArgv: [binary, ...runEmbeddedArgs],
});
spawnSyncAndExitWithoutError(
binary,
[ '--', ...buildSnapshotArgs ],
{ cwd: tmpdir.path },
{});
spawnSyncAndExitWithoutError(
binary,
[ '--', ...runSnapshotArgs ],
{ cwd: tmpdir.path },
{
stdout(output) {
assert.deepStrictEqual(JSON.parse(output), {
originalArgv: [binary, '__node_anonymous_main', ...buildSnapshotExecArgs],
currentArgv: [binary, ...runSnapshotExecArgs],
});
return true;
},
});
}

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

fs.rmSync(blobPath, { force: true });
assert.strictEqual(child_process.spawnSync(binary, [
'--', ...buildSnapshotArgs,
], {
cwd: tmpdir.path,
}).status, 0);
assert.strictEqual(
child_process.spawnSync(binary, ['--', '--embedder-snapshot-blob', blobPath]).status,
0);

spawnSyncAndExitWithoutError(
binary,
[ '--', ...buildSnapshotArgs ],
{ cwd: tmpdir.path },
{});
spawnSyncAndExitWithoutError(
binary,
[ '--', ...runEmbeddedArgs ],
{ cwd: tmpdir.path },
{});
}

0 comments on commit 1fffda5

Please sign in to comment.