test: update test-require-json.js

[alejandronanez]

Used assert.throws instead of assert.ok in test/parallel/test-require-json.

Checklist

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

[Trott]

Welcome @alejandronanez and thanks for the pull request. I imagine the idea here was to replace the try/catch. This change as it stands merely complicates the test. If you're at Code & Learn and you're not sure how to proceed, ask a mentor for some help! Thanks!

[alejandronanez]

Hey @Trott thanks for chiming in!

Actually this change was made based on the feedback from @ErickWendel at the Code & Learn session, fwiw, this was the suggested solution for this change:

The test contains a try / catch which does not verify that the error is actually triggered. Please refactor this by using the correct assert function instead.

How should I proceed from here then?

Thanks again :)!

[Trott]

Actually this change was made based on the feedback from @ErickWendel at the Code & Learn session, fwiw, this was the suggested solution for this change:

The test contains a try / catch which does not verify that the error is actually triggered. Please refactor this by using the correct assert function instead.

How should I proceed from here then?

Replace the entire try/catch block with an equivalent assert.throws() call. It is not a case of replacing assert.ok() with assert.throws().

[ErickWendel]

haha @alejandronanez sorry, I told you to put the assert.throws but I forgot to tell you to remove the entire try/catch. It was busy with many questions during the session. If you wanna some help, feel free to reach me :D

[Trott]

As this change currently stands, it does not merely complicate the test. It also invalidates it! The test will always pass. The error is no longer checked, other than to confirm that throw err throws err. It's easy to see how the change can be improved on and be totally acceptable, but right now, it complicates the code and invalidates the test.

[alejandronanez]

Hello @Trott, I made some updates to the test.

Let me know what you think, thanks!

[Trott]

Hello @Trott, I made some updates to the test.

Let me know what you think, thanks!

We're getting close!

[nodejs-github-bot]

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

[BridgeAR]

LGTM with the comment addressed.

[alejandronanez]

wow I completely missed this conversation! I already fixed the lint errors in edac77982b56b5a358361048ad6166e7f2f1c725. Thanks y'all for your feedback

[Trott]

This change means that we are no longer checking that the path that failed is included in the error message. Is that OK? @BridgeAR @addaleax @trivikr @jasnell

[Trott]

Rebased against master, added the regexp back so we're still checking that the message correctly reports the path of the file that caused the problem, and force-pushed.

@jasnell @trivikr @addaleax @BridgeAR PTAL to confirm that this is still to your liking. Thanks!

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[Trott]

Landed in c3b2111.

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)