test: add module require tests for certain package.json errors #23285
Conversation
test for unusual error cases: verify that module require() falls back to index if package.json names a missing file and throws an error if package.json is unparseable.
| } catch (e) { | ||
| unparseableErrorThrown = true; | ||
| assert.strictEqual(/^Error parsing .*/.test(e.message), true); | ||
| } |
There was a problem hiding this comment.
Lines 109 through 115 and line 330 should probably be removed and replaced with a call to assert.throws():
assert.throws(
() => { require('../fixtures/packages/unparseable'); },
/^Error parsing /
);
|
Not a big deal but maybe for next time something like this arises: Might be a good idea to split these two additions into separate commits. If one test case ends up being reverted or something, it won't take the other test one with it. |
|
What motivated adding these tests? Were you reviewing coverage statistics and trying to improve coverage? If not, how did you determine that there wasn't an existing test case for these situations? |
|
Some unusual require() behavior was noticed in a while back in #22464 and we updated the docs. I took a look at the code and the test coverage and did not see tests for some of the edge cases. |
|
@nodejs/testing |
| // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE | ||
| // USE OR OTHER DEALINGS IN THE SOFTWARE. | ||
|
|
||
| exports.ok = 'ok'; |
There was a problem hiding this comment.
Oh, can you remove all the copyright boilerplate for this file? That copyright boilerplate is only necessary for files that already have it or new files that are substantially derived from existing files with the copyright. Since this is just one line, I think we're in the clear.
There was a problem hiding this comment.
(Uh, unless you did in fact copy this from an existing file, in which case, leave the copyright. :-D )
There was a problem hiding this comment.
:) I left it since it is identical to several other files (so that the tests are consistent in design) and at least arguably is a copy of them.
There was a problem hiding this comment.
:) I left it since it is identical to several other files (so that the tests are consistent in design) and at least arguably is a copy of them.
Sounds good to me. Thanks!
There was a problem hiding this comment.
New files should not contain the notice.
There was a problem hiding this comment.
New files that, like this one, are exact copies of existing files should preserve the notice.
This file is an exact copy of test/fixtures/packages/main/package-main-module.js and test/fixtures/packages/main-index/package-main-module/index.js.
|
Landed in 13e6e01. Thanks! |
test for unusual error cases: verify that module require() falls back to index if package.json names a missing file and throws an error if package.json is unparseable. PR-URL: nodejs#23285 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
test for unusual error cases: verify that module require() falls back to index if package.json names a missing file and throws an error if package.json is unparseable. PR-URL: #23285 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
test for unusual error cases: verify that module require() falls back to index if package.json names a missing file and throws an error if package.json is unparseable. PR-URL: #23285 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
test for unusual error cases: verify that module require
falls back to index if package.json names a missing file and
throws an error if package.json is unparseable.
Checklist
make -j4 test(UNIX) passes