GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — test: fix argument computation in embedtest by joyeecheung · Pull Request #49506 · nodejs/node Conversation
|
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
9017e9d to
8b9b658
Compare
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Can I have some reviews please? @nodejs/startup @nodejs/cpp-reviewers |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
@nodejs/build-files @nodejs/cpp-reviewers can I get some reviews please? |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
| if (snapshot) { // Deserializing snapshot | ||
| loadenv_ret = node::LoadEnvironment(env, node::StartExecutionCallback{}); | ||
| } else { | ||
| } else if (is_building_snapshot) { |
There was a problem hiding this comment.
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 (
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; | |
| } |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
joyeecheung
Sep 15, 2023
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
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
Line 24 in 6a489df
| The full code can be found [in the Node.js source tree][embedtest.cc]. |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
To avoid capturing build machine states into the snapshot PR-URL: #49506 Fixes: #49501 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]>
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]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
joyeecheung
commented
Sep 17, 2023
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
|
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. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
To avoid capturing build machine states into the snapshot PR-URL: #49506 Fixes: #49501 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]>
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]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
Reviewers
+1 more reviewer
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
embedtest doesn't actually run any code
Uh oh!
There was an error while loading. Please reload this page.
5 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.