Skip to content

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
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

@gabrielschulhofgabrielschulhof commented May 27, 2020

Copy link
Copy Markdown
Contributor

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

@gabrielschulhof

gabrielschulhof commented May 27, 2020

Copy link
Copy Markdown
ContributorAuthor

@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

legendecas commented May 27, 2020

Copy link
Copy Markdown
Member

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

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

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

gabrielschulhof commented May 27, 2020

Copy link
Copy Markdown
ContributorAuthor

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

@gabrielschulhof

gabrielschulhof commented May 27, 2020

Copy link
Copy Markdown
ContributorAuthor

@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?

legendecas
legendecas reviewed Jun 8, 2020
View reviewed changes
Comment threadtest/threadsafe_function/threadsafe_function_sum.cc
mhdawson
mhdawson approved these changes Jun 9, 2020
View reviewed changes

@mhdawsonmhdawson 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.

LGTM

@mhdawson

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.

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

gabrielschulhof commented Jul 22, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

CI:

VersionJobStatus
v10.xhttps://ci.nodejs.org/job/node-test-node-addon-api-new/2486/✓
v12.xhttps://ci.nodejs.org/job/node-test-node-addon-api-new/2487/✗
v14.xhttps://ci.nodejs.org/job/node-test-node-addon-api-new/2488/✓
v15.xhttps://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
Gabriel Schulhof
test: run in both a child process and a worker thread
46e835f
Re: nodejs#730
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Gabriel Schulhof
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.
Gabriel Schulhof
add --experimental-worker
16cc763
@gabrielschulhof

gabrielschulhof commented Jul 22, 2020

Copy link
Copy Markdown
ContributorAuthor

Rebased.

@gabrielschulhof

gabrielschulhof commented Jul 22, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

CI:

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

@legendecaslegendecas mentioned this pull request Dec 15, 2020
Closed
@legendecas

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
legendecas reviewed Dec 15, 2020
View reviewed changes
Comment threadtest/index.js
}

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

@legendecaslegendecasDec 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.

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 mainJanuary 26, 2021 22:43
@gabrielschulhof
gabrielschulhof deleted the run-tests-in-workers2 branch February 1, 2021 16:39
@gabrielschulhof
gabrielschulhof restored the run-tests-in-workers2 branch February 1, 2021 16:41
@mhdawson

mhdawson commented Feb 1, 2021

Copy link
Copy Markdown
Member

@gabrielschulhof did you mean to close this?

@mhdawsonmhdawson reopened this Feb 1, 2021
@gabrielschulhof
gabrielschulhof deleted the run-tests-in-workers2 branch March 7, 2021 18:44
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@legendecaslegendecaslegendecas approved these changes
@mhdawsonmhdawsonmhdawson 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

@gabrielschulhof@legendecas@anfilat@mhdawson