| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
The modified test is failing in the Travis CI run. |
Sorry, something went wrong.
|
I'll take a look at it. Is there a way to check that locally? |
Sorry, something went wrong.
|
make test will run the test suite locally. Since you've only changed on test, node test/js-native-api/test_bigint/test.js should be enough. |
Sorry, something went wrong.
There was a problem hiding this comment.
I personally am -0 for this change. I would actually like to move more common.expectsError calls to use assert.throws. But it's not worth the churn to actually change them.
Sorry, something went wrong.
|
I'm with @BridgeAR on this. We should favor assert.throws() and avoid people having to learn our custom module where possible. If there are features in common.expectsError() that aren't in assert.throws(), we can consider adding them. |
Sorry, something went wrong.
|
It would be helpful to add some documentation to common.expectsError() to indicate when it should be used (and to direct test writers to use assert.throws() in the other cases). |
Sorry, something went wrong.
|
@mpark86 thank you very much for your contribution! I'll close this PR though, due to the reasons mentioned above. I opened #31092 to refactor common.expectsError() in a way that it is only possible to use as callback function. That way the confusion about it's usage will also be solved. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Modified test/js-native-api/test_bigint/test.js to use common.expectsError instead of assert.throws.