Skip to content

test: cover property n-api null cases - #31488

Closed
gabrielschulhof wants to merge 1 commit into
nodejs:masterfrom
gabrielschulhof:test-null-for-object-properties
Closed

test: cover property n-api null cases#31488
gabrielschulhof wants to merge 1 commit into
nodejs:masterfrom
gabrielschulhof:test-null-for-object-properties

Conversation

@gabrielschulhof

@gabrielschulhofgabrielschulhof commented Jan 24, 2020

Copy link
Copy Markdown
Contributor

Add test coverage for passing NULL to each parameter of
napi.*(propert|element). In the case of napi_define_properties also
test setting various initializer fields to NULL.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Jan 24, 2020
@gabrielschulhofgabrielschulhof added the node-api Issues and PRs related to the Node-API. label Jan 24, 2020
@gabrielschulhof
gabrielschulhofforce-pushed the test-null-for-object-properties branch 2 times, most recently from f7b7348 to f78645dCompareJanuary 24, 2020 02:59
devnexen
devnexen approved these changes Jan 24, 2020
View reviewed changes
Comment threadtest/js-native-api/test_object/test_null.c Outdated
Comment threadtest/js-native-api/test_object/test_null.c Outdated
Gabriel Schulhof
test: cover property n-api null cases
5f048fd
Add test coverage for passing `NULL` to each parameter of
`napi.*(propert|element)` and `napi_set_prototype`. In the case of
`napi_define_properties` also test setting various initializer fields
to `NULL`.
@gabrielschulhof
gabrielschulhofforce-pushed the test-null-for-object-properties branch from f78645d to 5f048fdCompareJanuary 24, 2020 15:41
@gabrielschulhof

gabrielschulhof commented Jan 24, 2020

Copy link
Copy Markdown
ContributorAuthor

@devnexen done.

@nodejs-github-bot

nodejs-github-bot commented Jan 26, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28675/

@TrottTrott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 26, 2020
@Trott

Trott commented Jan 26, 2020

Copy link
Copy Markdown
Member

Landed in 42995a3

@TrottTrott closed this Jan 26, 2020
Trott pushed a commit that referenced this pull request Jan 26, 2020
Gabriel Schulhof@Trott
test: cover property n-api null cases
42995a3
Add test coverage for passing `NULL` to each parameter of
`napi.*(propert|element)` and `napi_set_prototype`. In the case of
`napi_define_properties` also test setting various initializer fields
to `NULL`.
PR-URL: #31488
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
Gabriel Schulhof@codebytere
test: cover property n-api null cases
c4a2f94
Add test coverage for passing `NULL` to each parameter of
`napi.*(propert|element)` and `napi_set_prototype`. In the case of
`napi_define_properties` also test setting various initializer fields
to `NULL`.
PR-URL: #31488
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebyterecodebytere mentioned this pull request Feb 17, 2020
Merged
@codebytere

codebytere commented Mar 15, 2020

Copy link
Copy Markdown
Member

@gabrielschulhof this seems to have caused a deterministic error in v12.x; is it intended to go back there? If yes, can you please open a manual backport? I've set the label but feel free to update it

Details
Error: Command failed: /Users/codebytere/Developer/node/out/Release/node /Users/codebytere/Developer/node/deps/npm/node_modules/node-gyp/bin/node-gyp.js rebuild --directory=/Users/codebytere/Developer/node/test/js-native-api/test_object
../test_null.c:321:23: warning: implicit declaration of function 'napi_get_all_property_names' is invalid in C99 [-Wimplicit-function-declaration]
napi_get_all_property_names(NULL,
^
../test_null.c:323:51: error: use of undeclared identifier 'napi_key_own_only'
napi_key_own_only,
^
../test_null.c:324:51: error: use of undeclared identifier 'napi_key_writable'
napi_key_writable,
^
../test_null.c:325:51: error: use of undeclared identifier 'napi_key_keep_numbers'
napi_key_keep_numbers,
^
../test_null.c:330:31: error: use of undeclared identifier 'napi_key_own_only'
napi_key_own_only,
^
../test_null.c:331:31: error: use of undeclared identifier 'napi_key_writable'
napi_key_writable,
^
../test_null.c:332:31: error: use of undeclared identifier 'napi_key_keep_numbers'
napi_key_keep_numbers,
^
../test_null.c:338:31: error: use of undeclared identifier 'napi_key_own_only'
napi_key_own_only,
^
../test_null.c:339:31: error: use of undeclared identifier 'napi_key_writable'
napi_key_writable,
^
../test_null.c:340:31: error: use of undeclared identifier 'napi_key_keep_numbers'
napi_key_keep_numbers,
^
1 warning and 9 errors generated.
make[2]: *** [Release/obj.target/test_object/test_null.o] Error 1
at ChildProcess.exithandler (child_process.js:303:12)
at ChildProcess.emit (events.js:311:20)
at maybeClose (internal/child_process.js:1021:16)
at Process.ChildProcess._handle.onexit (internal/child_process.js:286:5) {
killed: false,
code: 1,
signal: null,
cmd: '/Users/codebytere/Developer/node/out/Release/node /Users/codebytere/Developer/node/deps/npm/node_modules/node-gyp/bin/node-gyp.js rebuild --directory=/Users/codebytere/Developer/node/test/js-native-api/test_object',
stdout: ' CC(target) Release/obj.target/test_object/../common.o\n' +
' CC(target) Release/obj.target/test_object/../entry_point.o\n' +
' CC(target) Release/obj.target/test_object/test_null.o\n',
stderr: "../test_null.c:321:23: warning: implicit declaration of function 'napi_get_all_property_names' is invalid in C99 [-Wimplicit-function-declaration]\n" +
' napi_get_all_property_names(NULL,\n' +
' ^\n' +
"../test_null.c:323:51: error: use of undeclared identifier 'napi_key_own_only'\n" +
' napi_key_own_only,\n' +
' ^\n' +
"../test_null.c:324:51: error: use of undeclared identifier 'napi_key_writable'\n" +
' napi_key_writable,\n' +
' ^\n' +
"../test_null.c:325:51: error: use of undeclared identifier 'napi_key_keep_numbers'\n" +
' napi_key_keep_numbers,\n' +
' ^\n' +
"../test_null.c:330:31: error: use of undeclared identifier 'napi_key_own_only'\n" +
' napi_key_own_only,\n' +
' ^\n' +
"../test_null.c:331:31: error: use of undeclared identifier 'napi_key_writable'\n" +
' napi_key_writable,\n' +
' ^\n' +
"../test_null.c:332:31: error: use of undeclared identifier 'napi_key_keep_numbers'\n" +
' napi_key_keep_numbers,\n' +
' ^\n' +
"../test_null.c:338:31: error: use of undeclared identifier 'napi_key_own_only'\n" +
' napi_key_own_only,\n' +
' ^\n' +
"../test_null.c:339:31: error: use of undeclared identifier 'napi_key_writable'\n" +
' napi_key_writable,\n' +
' ^\n' +
"../test_null.c:340:31: error: use of undeclared identifier 'napi_key_keep_numbers'\n" +
' napi_key_keep_numbers,\n' +
' ^\n' +
'1 warning and 9 errors generated.\n' +
'make[2]: *** [Release/obj.target/test_object/test_null.o] Error 1\n'
}
make[1]: *** [test/js-native-api/.buildstamp] Error 1
make[1]: *** Waiting for unfinished jobs....

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 2, 2020
Gabriel Schulhof
test: cover property n-api null cases
3cb02e5
Add test coverage for passing `NULL` to each parameter of
`napi.*(propert|element)` and `napi_set_prototype`. In the case of
`napi_define_properties` also test setting various initializer fields
to `NULL`.
PR-URL: nodejs#31488
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@gabrielschulhof

gabrielschulhof commented Apr 21, 2020

Copy link
Copy Markdown
ContributorAuthor

@codebytere please see #32482 which backports the remaining bits of N-API 6 to v12.x.

@gabrielschulhof
gabrielschulhof deleted the test-null-for-object-properties branch April 21, 2020 02:58
@targostargos added backport-open-v12.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v12.x labels Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
Gabriel Schulhof@targos
test: cover property n-api null cases
364fcaf
Add test coverage for passing `NULL` to each parameter of
`napi.*(propert|element)` and `napi_set_prototype`. In the case of
`napi_define_properties` also test setting various initializer fields
to `NULL`.
Backport-PR-URL: nodejs#32482
PR-URL: nodejs#31488
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
Gabriel Schulhof@targos
test: cover property n-api null cases
02ec03c
Add test coverage for passing `NULL` to each parameter of
`napi.*(propert|element)` and `napi_set_prototype`. In the case of
`napi_define_properties` also test setting various initializer fields
to `NULL`.
Backport-PR-URL: #32482
PR-URL: #31488
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targostargos mentioned this pull request May 2, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes
+1 more reviewer
@devnexendevnexendevnexen approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

node-apiIssues and PRs related to the Node-API.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@gabrielschulhof@nodejs-github-bot@Trott@codebytere@devnexen@targos