Skip to content

n-api: test and doc napi_throw() of a primitive#20428

Closed
gabrielschulhof wants to merge 1 commit intonodejs:masterfrom
gabrielschulhof:309-test-napi-throw-primitive
Closed

n-api: test and doc napi_throw() of a primitive#20428
gabrielschulhof wants to merge 1 commit intonodejs:masterfrom
gabrielschulhof:309-test-napi-throw-primitive

Conversation

@gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Apr 30, 2018

Ensure that napi_throw() is able to throw a primitive value, and
document that it is able to throw any JavaScript value.

Fixes: nodejs/abi-stable-node#309

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

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 30, 2018

Not sure if the line above should also be corrected:
`[in] error`: The `napi_value` for the `Error` to be thrown. ->
`[in] error`: The `napi_value` for the error to be thrown.

@vsemozhetbyt vsemozhetbyt added dont-land-on-v4.x node-api Issues and PRs related to the Node-API. labels Apr 30, 2018
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Apr 30, 2018

@vsemozhetbyt good point! I'll change it.

mhdawson
mhdawson approved these changes Apr 30, 2018
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM once the line above is updated as well.

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Apr 30, 2018

I actually changed it to

- `[in] error`: The JavaScript value to be thrown.

@mhdawson
Copy link
Member

mhdawson commented Apr 30, 2018

k, good.

@gabrielschulhof gabrielschulhof force-pushed the 309-test-napi-throw-primitive branch from 4c9501a to d671d11 Compare April 30, 2018 16:07
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 30, 2018

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

@vsemozhetbyt vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 30, 2018
n-api: test and doc napi_throw() of a primitive
e11e3d9 
Ensure that napi_throw() is able to throw a primitive value, and
document that it is able to throw any JavaScript value.

Fixes: nodejs/abi-stable-node#309
@gabrielschulhof gabrielschulhof force-pushed the 309-test-napi-throw-primitive branch from d671d11 to e11e3d9 Compare April 30, 2018 18:19
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Apr 30, 2018

I fixed a warning during the CI-lite. I'd actually like to run a full CI, because this includes code as well as doc: https://ci.nodejs.org/job/node-test-pull-request/14600/

@gabrielschulhof gabrielschulhof deleted the 309-test-napi-throw-primitive branch May 2, 2018 13:22
gabrielschulhof pushed a commit that referenced this pull request May 2, 2018
n-api: test and doc napi_throw() of a primitive
bdf5be9 
Ensure that napi_throw() is able to throw a primitive value, and
document that it is able to throw any JavaScript value.

Fixes: nodejs/abi-stable-node#309
PR-URL: #20428
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented May 2, 2018

Landed in bdf5be9.

@BridgeAR
Copy link
Member

BridgeAR commented May 3, 2018 •
edited
Loading

I comment on the commit itself but here for convenience:

These tests do not test anything besides that the function call throws. The value as error validation does not verify anything. It is a no-op.

When I check for throwArbitrary it is actually not even showing up as property on test_error? So I get an test_error.throwArbitrary is not a function error.

I get this back when running Object.getOwnPropertyNames(test_error)

[ 'checkError',
  'throwExistingError',
  'throwError',
  'throwRangeError',
  'throwTypeError',
  'throwErrorCode',
  'throwRangeErrorCode',
  'throwTypeErrorCode',
  'createError',
  'createRangeError',
  'createTypeError',
  'createErrorCode',
  'createRangeErrorCode',
  'createTypeErrorCode' ]

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented May 3, 2018

@BridgeAR you're right! I guess assert.throws() cannot be used for arbitrary thrown values. I'll simply do the try/catch by hand.

As for the missing property, that's weird. throwArbitrary is in the list for me.

@gabrielschulhof gabrielschulhof mentioned this pull request May 3, 2018
Closed
3 tasks
@BridgeAR
Copy link
Member

BridgeAR commented May 3, 2018

@gabrielschulhof seems like the addons were not properly rebuild on my machine earlier. That is why I had issues with the missing property. I somehow had a weird state and had to explicitly reset everything addon related before they wanted to recompile.

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented May 3, 2018 •
edited
Loading

@BridgeAR I'm glad it's all good now :)

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request May 4, 2018
test: fix up N-API error test
cfaec6e 
Replace assert.throws() with an explicit try/catch in order to catch
the thrown value and be able to compare it strictly to an expected
value.

Re: nodejs#20428 (comment)
MylesBorins pushed a commit that referenced this pull request May 4, 2018
@MylesBorins
n-api: test and doc napi_throw() of a primitive
ad793ab 
Ensure that napi_throw() is able to throw a primitive value, and
document that it is able to throw any JavaScript value.

Fixes: nodejs/abi-stable-node#309
PR-URL: #20428
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this pull request May 5, 2018
@addaleax
test: fix up N-API error test
3b60fc2 
Replace assert.throws() with an explicit try/catch in order to catch
the thrown value and be able to compare it strictly to an expected
value.

Re: #20428 (comment)

PR-URL: #20487
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 8, 2018
@MylesBorins
test: fix up N-API error test
6966c04 
Replace assert.throws() with an explicit try/catch in order to catch
the thrown value and be able to compare it strictly to an expected
value.

Re: #20428 (comment)

PR-URL: #20487
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
Merged
MylesBorins pushed a commit that referenced this pull request May 8, 2018
@MylesBorins
test: fix up N-API error test
21f0577 
Replace assert.throws() with an explicit try/catch in order to catch
the thrown value and be able to compare it strictly to an expected
value.

Re: #20428 (comment)

PR-URL: #20487
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 9, 2018
@MylesBorins
test: fix up N-API error test
70b2e16 
Replace assert.throws() with an explicit try/catch in order to catch
the thrown value and be able to compare it strictly to an expected
value.

Re: #20428 (comment)

PR-URL: #20487
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjihrig cjihrig cjihrig approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. node-api Issues and PRs related to the Node-API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Clarify behaviour of napi_throw()

5 participants

@gabrielschulhof @vsemozhetbyt @mhdawson @BridgeAR @cjihrig