Skip to content

test: fix up N-API error test - #20487

Closed
gabrielschulhof wants to merge 1 commit into
nodejs:masterfrom
gabrielschulhof:n-api-fixup-test-error
Closed

test: fix up N-API error test#20487
gabrielschulhof wants to merge 1 commit into
nodejs:masterfrom
gabrielschulhof:n-api-fixup-test-error

Conversation

@gabrielschulhof

Copy link
Copy Markdown
Contributor

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)

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 dont-land-on-v4.x node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels May 3, 2018

@BridgeARBridgeAR left a comment

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.

LGTM, I suggest to keep assert.throws nevertheless.

Comment threadtest/addons-napi/test_error/test.js 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.

It is possible to use assert.throws when using the following notation:

assert.throws(()=>test_error.throwArbitrary(value),(err)=>{assert.strictEqual(err,value);returntrue;});

@gabrielschulhof

Copy link
Copy Markdown
ContributorAuthor

@BridgeAR I updated the check as you suggested.

@mhdawsonmhdawson left a comment

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.

LGTM

@gabrielschulhof

Copy link
Copy Markdown
ContributorAuthor

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)
@gabrielschulhof
gabrielschulhofforce-pushed the n-api-fixup-test-error branch from cc09cb2 to cfaec6eCompareMay 4, 2018 02:44
@gabrielschulhof

Copy link
Copy Markdown
ContributorAuthor

sigh ... linter ... new CI: https://ci.nodejs.org/job/node-test-pull-request/14658/

@BridgeARBridgeAR added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels May 4, 2018
@BridgeAR

Copy link
Copy Markdown
Member

Please +1 if you are fine with fast tracking.

@addaleax

Copy link
Copy Markdown
Member

Landed in 3b60fc2

@addaleaxaddaleax closed this May 5, 2018
addaleax pushed a commit that referenced this pull request May 5, 2018
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
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>
@MylesBorinsMylesBorins mentioned this pull request May 8, 2018
MylesBorins pushed a commit that referenced this pull request May 8, 2018
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
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>
@gabrielschulhof
gabrielschulhof deleted the n-api-fixup-test-error branch May 16, 2018 15:20
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.fast-trackPRs that do not need to wait for 48 hours to land.node-apiIssues and PRs related to the Node-API.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@gabrielschulhof@BridgeAR@addaleax@jasnell@cjihrig@mhdawson@trivikr@nodejs-github-bot