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: fix ineffective error tests #27333

Closed
wants to merge 3 commits into from
Closed

test: fix ineffective error tests #27333

wants to merge 3 commits into from

Conversation

shisama
Copy link
Contributor

@shisama shisama commented Apr 21, 2019

Fix tests whether errors are thrown correctly
because they are successful when error doesn't get thrown.

fix #26385

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

@nodejs-github-bot
Copy link
Collaborator

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3316

@nodejs-github-bot nodejs-github-bot added addons Issues and PRs related to native addons. test Issues and PRs related to the tests. labels Apr 21, 2019
test: fix ineffective error tests
52fb259 
Fix tests whether errors are thrown correctly
because they are successful when error doesn't get thrown.
@shisama shisama force-pushed the test-ineffective-errors branch from aa46727 to 52fb259 Compare April 21, 2019 14:52
BridgeAR
BridgeAR approved these changes Apr 21, 2019
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM while I would prefer to use assert.throws(fn, errorFunction) (or assert.rejects() for promises) instead of try / catch in combination with threw or assert.fail(). That way each test is nicely encapsulated.

@shisama shisama force-pushed the test-ineffective-errors branch 3 times, most recently from 3d63790 to c862bd1 Compare April 21, 2019 15:51
@shisama shisama force-pushed the test-ineffective-errors branch from c862bd1 to 823bfbe Compare April 21, 2019 16:45
@shisama
Copy link
Contributor Author

shisama commented Apr 21, 2019

@BridgeAR Thank you for your review. I fix this PR to use assert.throws or assert.rejects instead of try / catch. Would you please review this again?

BridgeAR
BridgeAR reviewed Apr 21, 2019
View reviewed changes
test/parallel/test-assert.js Show resolved Hide resolved
test/parallel/test-vm-codegen.js Outdated Show resolved Hide resolved
test/parallel/test-assert.js Show resolved Hide resolved
BridgeAR
BridgeAR approved these changes Apr 23, 2019
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Still LGTM. Thanks for following up on the suggestions.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 23, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@danbev
Copy link
Contributor

danbev commented Apr 24, 2019

Landed in fb3f600.

@danbev danbev closed this Apr 24, 2019
danbev pushed a commit that referenced this pull request Apr 24, 2019
@danbev
test: fix ineffective error tests
fb3f600 
Fix tests whether errors are thrown correctly
because they are successful when error doesn't get thrown.

PR-URL: #27333
Fixes: #26385
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@shisama shisama deleted the test-ineffective-errors branch April 24, 2019 12:43
targos pushed a commit that referenced this pull request Apr 27, 2019
@targos
test: fix ineffective error tests
f0b2992 
Fix tests whether errors are thrown correctly
because they are successful when error doesn't get thrown.

PR-URL: #27333
Fixes: #26385
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@targos targos mentioned this pull request Apr 27, 2019
Merged
@ChALkeR
Copy link
Member

ChALkeR commented May 22, 2019

Thanks for doing this! =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
addons Issues and PRs related to native addons. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tests: review and fix ineffective error tests
6 participants
@shisama @nodejs-github-bot @danbev @ChALkeR @lpinca @BridgeAR