Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: cover property n-api null cases #31488

Closed
wants to merge 1 commit into from
Closed

test: cover property n-api null cases #31488

wants to merge 1 commit into from

Conversation

gabrielschulhof
Copy link
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-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 24, 2020
@gabrielschulhof gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Jan 24, 2020
@gabrielschulhof gabrielschulhof force-pushed the test-null-for-object-properties branch 2 times, most recently from f7b7348 to f78645d Compare January 24, 2020 02:59
devnexen
devnexen approved these changes Jan 24, 2020
View reviewed changes
test/js-native-api/test_object/test_null.c Outdated Show resolved Hide resolved
test/js-native-api/test_object/test_null.c Outdated Show resolved Hide resolved
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
Copy link
Contributor Author

@devnexen done.

@nodejs-github-bot
Copy link
Collaborator

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

@Trott Trott 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
Copy link
Member

Trott commented Jan 26, 2020

Landed in 42995a3

@Trott Trott closed this Jan 26, 2020
Trott pushed a commit that referenced this pull request Jan 26, 2020
@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
@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>
@codebytere codebytere mentioned this pull request Feb 17, 2020
Merged
@codebytere
Copy link
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

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
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
Copy link
Contributor Author

@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
@targos targos 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
@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
@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>
@targos targos mentioned this pull request May 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@devnexen devnexen devnexen approved these changes

Assignees
No one assigned
Labels
node-api Issues and PRs related to the Node-API. test Issues 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