Conversation
|
@nodejs/testing @nodejs/modules-active-members |
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
Thank you so much @zeckson for working on improving this coverage.
| common.mustCall((err, stdout) => { | ||
| assert.ifError(err); | ||
| assert.strictEqual(stdout, 'undefined\n'); | ||
| })); |
There was a problem hiding this comment.
We probably don't need this test, but there's nothing wrong with including it.
There was a problem hiding this comment.
It may be valuable to capture the default behavior. If we'd ever (accidentally) flip the default type, this would fail. Though realistically a lot of tests would fail then...
|
|
||
| // Assert that import.meta is defined in ESM | ||
| child.exec( | ||
| `${nodejs} ${execOptions} --eval "console.log(typeof import.meta);"`, |
There was a problem hiding this comment.
It could also be worth making this a test of import.meta.url. Note that such a test would be the exact test needed to fix #28160 (comment).
There was a problem hiding this comment.
I would be a bit concerned that it would increase the test scope. E.g. this test is meant to verify that the source is run as a module. If we change the exact string in import.meta.url in the future, it would break this test even though the source is still running as a module. If we want to test specific behavior ("relative imports work in --eval"), I would prefer a dedicated test for that.
|
@jkrems Does this look good to you? (No pressure or anything, obviously, but I'm just asking becauseI'd prefer to land it with more than one approval.) |
hybrist
left a comment
hybrist
left a comment
There was a problem hiding this comment.
Thanks a lot for these tests, this is great!
ZYSzys
added
author ready
PR-URL: nodejs#27956 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #27956 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #27956 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
8 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes