Skip to content

test: make cctest fixture use node::NewIsolate - #21419

Closed
danbev wants to merge 2 commits into
nodejs:masterfrom
danbev:cctest-fixture-newisolate
Closed

test: make cctest fixture use node::NewIsolate#21419
danbev wants to merge 2 commits into
nodejs:masterfrom
danbev:cctest-fixture-newisolate

Conversation

@danbev

Copy link
Copy Markdown
Contributor

This commit updates the gtest fixture to use node::NewIsolate instead of
creating a new V8 Isolate using v8::Isolate::New.

The motivation for this is that without calling node::NewIsolate the
various callbacks set on the isolate, for example AddMessageListener,
SetFatalErrorHandler etc, would not get set. I don't think this is the
expected behaviour and I ran into this when writing a new cctest.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Jun 20, 2018
@danbev

Copy link
Copy Markdown
ContributorAuthor

@danbev

Copy link
Copy Markdown
ContributorAuthor
node-test-commit-aix failure looks unrelated

console output:

03:24:59 Node.js major version: -103:24:59 Node.js version: null03:24:59 Running nodes03:24:59 Triggering node-test-commit-aix » aix61-ppc6403:55:03 Completed node-test-commit-aix » aix61-ppc64 FAILURE03:55:03 All downstream projects complete!03:55:03 Notifying upstream projects of job completion03:55:03 Finished: FAILURE

Comment threadtest/cctest/node_test_fixture.h Outdated

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.

This looks mildly questionable. Why not e.g.:

allocator = ArrayBufferUniquePtr(node::CreateArrayBufferAllocator(), &node::FreeArrayBufferAllocator);

(Might need a std::move().)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I used reset mainly to follow the pattern of the other unique_ptr's in that file but this would be much cleaner. I'll update shortly. Thanks

@danbev

Copy link
Copy Markdown
ContributorAuthor

danbev added 2 commits June 26, 2018 08:10
This commit updates the gtest fixture to use node::NewIsolate instead of
creating a new V8 Isolate using v8::Isolate::New.
The motivation for this is that without calling node::NewIsolate the
various callbacks set on the isolate, for example AddMessageListener,
SetFatalErrorHandler etc, would not get set. I don't think this is the
expected behaviour and I ran into this when writing a new cctest.
@danbev
danbevforce-pushed the cctest-fixture-newisolate branch from 934e969 to 1edab2dCompareJune 26, 2018 06:17
@danbev

Copy link
Copy Markdown
ContributorAuthor

@danbev

Copy link
Copy Markdown
ContributorAuthor

Landed in d6f7a32, and 8326bea.

@danbevdanbev closed this Jun 27, 2018
@danbev
danbev deleted the cctest-fixture-newisolate branch June 27, 2018 03:28
danbev added a commit that referenced this pull request Jun 27, 2018
This commit updates the gtest fixture to use node::NewIsolate instead of
creating a new V8 Isolate using v8::Isolate::New.
The motivation for this is that without calling node::NewIsolate the
various callbacks set on the isolate, for example AddMessageListener,
SetFatalErrorHandler etc, would not get set. I don't think this is the
expected behaviour and I ran into this when writing a new cctest.
PR-URL: #21419
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
danbev added a commit that referenced this pull request Jun 27, 2018
PR-URL: #21419
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request Jun 28, 2018
This commit updates the gtest fixture to use node::NewIsolate instead of
creating a new V8 Isolate using v8::Isolate::New.
The motivation for this is that without calling node::NewIsolate the
various callbacks set on the isolate, for example AddMessageListener,
SetFatalErrorHandler etc, would not get set. I don't think this is the
expected behaviour and I ran into this when writing a new cctest.
PR-URL: #21419
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request Jun 28, 2018
PR-URL: #21419
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@targostargos mentioned this pull request Jul 3, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@danbev@nodejs-github-bot@bnoordhuis