test: run interfering tests in their own process#1325
Closed
gabrielschulhof wants to merge 1 commit intonodejs:mainfrom
Closed
test: run interfering tests in their own process#1325gabrielschulhof wants to merge 1 commit intonodejs:mainfrom
gabrielschulhof wants to merge 1 commit intonodejs:mainfrom
Conversation
gabrielschulhof
force-pushed
the
remove-instance-data-interference
branch
from
b63f9e7 to
983ebd2
Compare
Member
|
Jenkins ci run on latest (main) - https://ci.nodejs.org/job/node-test-node-addon-api-new/7647/ jenkins ci run on earliest (16.x) - https://ci.nodejs.org/job/node-test-node-addon-api-new/7648/ |
legendecas
approved these changes
Jul 3, 2023
test/addon_data.cc
Outdated
| // 0. A boolean named "verbose" is stored in the instance data. The constructor | ||
| // for JS `VerboseIndicator` instances is also stored in the instance data. | ||
| // 0. The constructor for JS `VerboseIndicator` instances, which have a private | ||
| // member named "verbose" is stored in the instance data. |
Member
There was a problem hiding this comment.
Suggested change
| // member named "verbose" is stored in the instance data. | |
| // member named "verbose" stored in the instance data. |
Contributor
Author
There was a problem hiding this comment.
I meant to say that the constructor is stored on the instance data, so rather than remove the word "is", I need to put a comma in front of it.
test/common/index.js
Outdated
| // Capture the exit code and signal. | ||
| child | ||
| .on('exit', (code, signal) => maybeResolve({ code, signal })) | ||
| .on('close', () => maybeResolve()); |
Member
There was a problem hiding this comment.
'close' event is always emitted after the 'exit' event (https://nodejs.org/api/child_process.html#event-close). So this can be simplified as:
Suggested change
| .on('close', () => maybeResolve()); | |
| .on('close', (code, signal) => resolve({ code, signal })); |
Tests such as addon and addon_data both use SetInstanceData. Thus, they overwrite each other's instance data. We change them both to run in a child process such that they do not call SetInstanceData on init, but rather they return a JS function on init which, when called, produces the actual binding which needs SetInstanceData. We also introduce a utility function for launching a test in its own child process for the purpose of running tests that would otherwise interfere with each other.
gabrielschulhof
force-pushed
the
remove-instance-data-interference
branch
from
1de5ef4 to
38061b3
Compare
legendecas
approved these changes
Jul 7, 2023
Contributor
Author
gabrielschulhof
added a commit
that referenced
this pull request
Jul 8, 2023
Tests such as addon and addon_data both use SetInstanceData. Thus, they overwrite each other's instance data. We change them both to run in a child process such that they do not call SetInstanceData on init, but rather they return a JS function on init which, when called, produces the actual binding which needs SetInstanceData. We also introduce a utility function for launching a test in its own child process for the purpose of running tests that would otherwise interfere with each other. Signed-off-by: Gabriel Schulhof <[email protected]> PR-URL: #1325 Reviewed-By: Chengzhong Wu <[email protected]>
Contributor
Author
|
Landed in 414be9e. |
johnfrench3
pushed a commit
to johnfrench3/node-addon-api-git
that referenced
this pull request
Aug 11, 2023
Tests such as addon and addon_data both use SetInstanceData. Thus, they overwrite each other's instance data. We change them both to run in a child process such that they do not call SetInstanceData on init, but rather they return a JS function on init which, when called, produces the actual binding which needs SetInstanceData. We also introduce a utility function for launching a test in its own child process for the purpose of running tests that would otherwise interfere with each other. Signed-off-by: Gabriel Schulhof <[email protected]> PR-URL: nodejs/node-addon-api#1325 Reviewed-By: Chengzhong Wu <[email protected]>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tests such as addon and addon_data both use SetInstanceData. Thus, they overwrite each other's instance data. We change them both to run in a child process such that they do not call SetInstanceData on init, but rather they return a JS function on init which, when called, produces the actual binding which needs SetInstanceData.
We also introduce a utility function for launching a test in its own child process for the purpose of running tests that would otherwise interfere with each other.