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

test: run in both a child process and a worker thread by gabrielschulhof · Pull Request #738 · nodejs/node-addon-api · GitHub

/ node-addon-api Public

test: run in both a child process and a worker thread - #738

Closed
gabrielschulhof wants to merge 3 commits into
nodejs:mainfrom
gabrielschulhof:run-tests-in-workers2
ConversationCommits (3)ChecksFiles changed
Closed

test: run in both a child process and a worker thread#738
gabrielschulhof wants to merge 3 commits into
nodejs:mainfrom
gabrielschulhof:run-tests-in-workers2

Conversation

gabrielschulhof commented May 27, 2020

Copy link
Copy Markdown
Contributor

Re: #730
Signed-off-by: @gabrielschulhof

gabrielschulhof commented May 27, 2020

Copy link
Copy Markdown
Contributor Author

@anfilat I changed the way we run all tests to run on a worker thread as well as on the main process. I could not reproduce #730. Can you please test this code and tell me if I'm missing anything?

legendecas commented May 27, 2020

Copy link
Copy Markdown
Member

The CI failure might be related to nodejs/node#33508?

nick-erm mentioned this pull request May 27, 2020
Open

anfilat commented May 27, 2020

Copy link
Copy Markdown

@gabrielschulhof With my test I wanted to try use SetInstanceData/GetInstanceData in different threads at the same time. But the test was crashed by working different addons at same thread (addon_data and objectwrap). You new testrunner starts each addon at a new process.

gabrielschulhof commented May 27, 2020

Copy link
Copy Markdown
Contributor Author

@anfilat understood. In that case, that's probably a test case to add to Node.js.core.

gabrielschulhof commented May 27, 2020

Copy link
Copy Markdown
Contributor Author

@legendecas I had to fix the threadsafe_function_sum test because it was quitting before the promise was resolved. Can you please take another look?

gabrielschulhof force-pushed the run-tests-in-workers2 branch from 82bfab0 to 25b5985 Compare June 2, 2020 19:36
mhdawson approved these changes Jun 9, 2020
View reviewed changes

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

mhdawson removed request for KevinEady and NickNaso June 9, 2020 23:40

mhdawson commented Jun 9, 2020

Copy link
Copy Markdown
Member

This needs nodejs/node#33508 to be backported to 14.x and 12.x before it can land.

gabrielschulhof mentioned this pull request Jun 29, 2020
Closed
3 tasks

gabrielschulhof commented Jul 22, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

CI:

Version Job Status
v10.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2486/ ✓
v12.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2487/ ✗
v14.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2488/ ✓
v15.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2489/ ✗

We're getting some intermittent segfaults.

Gabriel Schulhof added 3 commits July 22, 2020 12:09
test: run in both a child process and a worker thread
46e835f 
Re: nodejs#730
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
test: fix tsfn sum test
7370b7f 
The TSFN sum test was not waiting for all TSFN calls to complete before
releasing the TSFN completely. The main thread must only release the
TSFN and resolve the deferred if all calls have completed.
add --experimental-worker
16cc763
gabrielschulhof force-pushed the run-tests-in-workers2 branch from 25b5985 to 16cc763 Compare July 22, 2020 19:11

gabrielschulhof commented Jul 22, 2020

Copy link
Copy Markdown
Contributor Author

Rebased.

gabrielschulhof commented Jul 22, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

CI:

Version Job Status
v10.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2490/ ✓
v12.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2491/ ✗
smartos18-64: running as worker segfaults ✗
v14.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2492/ ✓
v15.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2493/ ✓

legendecas mentioned this pull request Dec 15, 2020
Closed

legendecas commented Dec 15, 2020

Copy link
Copy Markdown
Member

@mhdawson: This needs nodejs/node#33508 to be backported to 14.x and 12.x before it can land.

nodejs/node#33508 has been backported to 14.5.0 and 12.18.3. @gabrielschulhof is there any other blocking issue for this one?

legendecas reviewed Dec 15, 2020
View reviewed changes
Comment thread test/index.js
}

testModules.forEach((name) => {
runOneChild(name, 'child');

legendecas Dec 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'd worried that all the cases were started at the same time. Can we wait for one case to finish then start another one?

Base automatically changed from master to main January 26, 2021 22:43
gabrielschulhof deleted the run-tests-in-workers2 branch February 1, 2021 16:39
gabrielschulhof restored the run-tests-in-workers2 branch February 1, 2021 16:41

mhdawson commented Feb 1, 2021

Copy link
Copy Markdown
Member

@gabrielschulhof did you mean to close this?

mhdawson reopened this Feb 1, 2021
gabrielschulhof deleted the run-tests-in-workers2 branch March 7, 2021 18:44
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

legendecas legendecas approved these changes
mhdawson mhdawson approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL