Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: fix argument computation in embedtest #49506

Closed
wants to merge 4 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Sep 5, 2023

bootstrap: do not expand argv1 for snapshots

To avoid capturing build machine states into the snapshot

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.

build: run embedtest using node executable

We should use the node executable to run this test, instead of
counting on embedtest, the binary being tested, as the test runner.
Otherwise bugs can go unnoticed if the embedtest binary itself
does not work correctly.

Fixes: #49501

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels Sep 5, 2023
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 6, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 6, 2023
@nodejs-github-bot
Copy link
Collaborator

To avoid capturing build machine states into the snapshot
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.
We should use the node executable to run this test, instead of
counting on embedtest, the binary being tested, as the test runner.
Otherwise bugs can go unnoticed if the embedtest binary itself
does not work correctly.
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

Can I have some reviews please? @nodejs/startup @nodejs/cpp-reviewers

test/embedding/embedtest.cc Outdated Show resolved Hide resolved
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 13, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 13, 2023
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

@nodejs/build-files @nodejs/cpp-reviewers can I get some reviews please?

loadenv_ret = node::LoadEnvironment(env, node::StartExecutionCallback{});
} else {
} else if (is_building_snapshot) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we sync the code in the embedding docs (

node/doc/api/embedding.md

Lines 112 to 166 in 6a489df

int RunNodeInstance(MultiIsolatePlatform* platform,
const std::vector<std::string>& args,
const std::vector<std::string>& exec_args) {
int exit_code = 0;
// Setup up a libuv event loop, v8::Isolate, and Node.js Environment.
std::vector<std::string> errors;
std::unique_ptr<CommonEnvironmentSetup> setup =
CommonEnvironmentSetup::Create(platform, &errors, args, exec_args);
if (!setup) {
for (const std::string& err : errors)
fprintf(stderr, "%s: %s\n", args[0].c_str(), err.c_str());
return 1;
}
Isolate* isolate = setup->isolate();
Environment* env = setup->env();
{
Locker locker(isolate);
Isolate::Scope isolate_scope(isolate);
HandleScope handle_scope(isolate);
// The v8::Context needs to be entered when node::CreateEnvironment() and
// node::LoadEnvironment() are being called.
Context::Scope context_scope(setup->context());
// Set up the Node.js instance for execution, and run code inside of it.
// There is also a variant that takes a callback and provides it with
// the `require` and `process` objects, so that it can manually compile
// and run scripts as needed.
// The `require` function inside this script does *not* access the file
// system, and can only load built-in Node.js modules.
// `module.createRequire()` is being used to create one that is able to
// load files from the disk, and uses the standard CommonJS file loader
// instead of the internal-only `require` function.
MaybeLocal<Value> loadenv_ret = node::LoadEnvironment(
env,
"const publicRequire ="
" require('node:module').createRequire(process.cwd() + '/');"
"globalThis.require = publicRequire;"
"require('node:vm').runInThisContext(process.argv[1]);");
if (loadenv_ret.IsEmpty()) // There has been a JS exception.
return 1;
exit_code = node::SpinEventLoop(env).FromMaybe(1);
// node::Stop() can be used to explicitly stop the event loop and keep
// further JavaScript from running. It can be called from any thread,
// and will act like worker.terminate() if called from another thread.
node::Stop(env);
}
return exit_code;
}
) too?

Copy link
Member Author

@joyeecheung joyeecheung Sep 15, 2023

Choose a reason for hiding this comment

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

Maybe, the code in there would continue to work, the snapshot capability and the snapshot testing code was added by #45888, so probably out of scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

The doc has a reference to this file in

The full code can be found [in the Node.js source tree][embedtest.cc].
, so I thought these were supposed to be in sync but this too lgtm if that's not needed

@nodejs-github-bot
Copy link
Collaborator

joyeecheung added a commit that referenced this pull request Sep 17, 2023
To avoid capturing build machine states into the snapshot

PR-URL: #49506
Fixes: #49501
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Sep 17, 2023
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>
joyeecheung added a commit that referenced this pull request Sep 17, 2023
We should use the node executable to run this test, instead of
counting on embedtest, the binary being tested, as the test runner.
Otherwise bugs can go unnoticed if the embedtest binary itself
does not work correctly.

PR-URL: #49506
Fixes: #49501
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@joyeecheung
Copy link
Member Author

joyeecheung commented Sep 17, 2023

CI was green although the bot still had trouble communicating the green result back to GitHub so it got stuck as "pending. Landed it manually in cdcb01a...1995eca.

ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
To avoid capturing build machine states into the snapshot

PR-URL: #49506
Fixes: #49501
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
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>
ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
We should use the node executable to run this test, instead of
counting on embedtest, the binary being tested, as the test runner.
Otherwise bugs can go unnoticed if the embedtest binary itself
does not work correctly.

PR-URL: #49506
Fixes: #49501
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Sep 28, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
To avoid capturing build machine states into the snapshot

PR-URL: nodejs#49506
Fixes: nodejs#49501
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
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: nodejs#49506
Fixes: nodejs#49501
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
We should use the node executable to run this test, instead of
counting on embedtest, the binary being tested, as the test runner.
Otherwise bugs can go unnoticed if the embedtest binary itself
does not work correctly.

PR-URL: nodejs#49506
Fixes: nodejs#49501
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

embedtest doesn't actually run any code
5 participants