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: #49506Fixes: #49501 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Author: joyeecheung

test/embedding/embedtest.cc

@@ -89,7 +89,7 @@ int RunNodeInstance(MultiIsolatePlatform* platform,
  snapshot_as_file = true;
  } elseif (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);
@@ -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());
  }
@@ -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{
+ } elseif (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.

test/embedding/test-embedding.js

@@ -3,7 +3,10 @@ const common = require('../common');
 constfixtures=require('../common/fixtures');
 consttmpdir=require('../common/tmpdir');
 constassert=require('assert');
-constchild_process=require('child_process');
+const{
+ spawnSyncAndExit,
+ spawnSyncAndExitWithoutError,
+}=require('../common/child_process');
 constpath=require('path');
 constfs=require('fs');
 
@@ -21,39 +24,54 @@ function resolveBuiltBinary(bin){
 
 constbinary=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,
+});
 
 constfixturePath=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,
+});
 
 functiongetReadFileCodeForPath(path){
 return`(require("fs").readFileSync(${JSON.stringify(path)}, "utf8"))`;
@@ -64,31 +82,49 @@ for (const extraSnapshotArgs of [[], ['--embedder-snapshot-as-file']]){
 // readSync + eval since snapshots don't support userland require() (yet)
 constsnapshotFixture=fixtures.path('snapshot','echo-args.js');
 constblobPath=tmpdir.resolve('embedder-snapshot.blob');
-constbuildSnapshotArgs=[
+constbuildSnapshotExecArgs=[
 `eval(${getReadFileCodeForPath(snapshotFixture)})`,'arg1','arg2',
+];
+constembedTestBuildArgs=[
 '--embedder-snapshot-blob',blobPath,'--embedder-snapshot-create',
  ...extraSnapshotArgs,
 ];
-construnEmbeddedArgs=[
-'--embedder-snapshot-blob',blobPath, ...extraSnapshotArgs,'arg3','arg4',
+constbuildSnapshotArgs=[
+ ...buildSnapshotExecArgs,
+ ...embedTestBuildArgs,
+];
+
+construnSnapshotExecArgs=[
+'arg3','arg4',
+];
+constembedTestRunArgs=[
+'--embedder-snapshot-blob',blobPath,
+ ...extraSnapshotArgs,
+];
+construnSnapshotArgs=[
+ ...runSnapshotExecArgs,
+ ...embedTestRunArgs,
 ];
 
 fs.rmSync(blobPath,{force: true});
-constchild=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);
-constspawnResult=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],
+});
+returntrue;
+},
+});
 }
 
 // Create workers and vm contexts after deserialization
@@ -99,14 +135,20 @@ for (const extraSnapshotArgs of [[], ['--embedder-snapshot-as-file']]){
 `eval(${getReadFileCodeForPath(snapshotFixture)})`,
 '--embedder-snapshot-blob',blobPath,'--embedder-snapshot-create',
 ];
+construnEmbeddedArgs=[
+'--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},
+{});
 }