test: add smoke tests for defer importing synthetic modules - #65537
MayaLekova wants to merge 1 commit into
Conversation
03ccd50 to
414cd49
Compare
|
@joyeecheung and @nicolo-ribaudo could you take a look please? Just a few smoke tests for synthetic modules. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #65537 +/- ##
==========================================
+ Coverage 90.21% 90.24% +0.03%
==========================================
Files 751 789 +38
Lines 253550 270476 +16926
Branches 47813 51751 +3938
==========================================
+ Hits 228733 244092 +15359
- Misses 16076 16868 +792
- Partials 8741 9516 +775 🚀 New features to boost your workflow:
|
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
The tested behavior looks correct to me.
For the node:fs, is there any way to test when evaluation happens? Maybe if one monkey-patches some built-ins before evaluating node:fs then it does something different? (I hope the answer is "no it's not possible")
If the builtin was to write something to the global namespace, that would make it easier. But I guess they don't do so, in order no to pollute the global namespace. Also there might be another builtin module that writes something on |
0302cad to
7f7855e
Compare
|
Suggestion for now - I can land those tests as they are currently, and then as a follow up:
Is there another mechanism in Node.js to expose internal details from C++ to JS? |
|
Can you import a module that's not in the snapshot (something like http should do), and use |
You can simply get to the |
Wouldn't this work only if |
7f7855e to
c6bb77a
Compare
Okay, I added a new version of the test that uses some internal machinery to obtain the @joyeecheung Please take another look, thanks! |
| // Load the http builtin module and check that it is evaluated. | ||
| let builtin = helpers.default.loadBuiltinModule('http'); | ||
| let wrap = builtin.getESMFacade(); | ||
| assert.strictEqual(wrap.getStatus(), kEvaluated); |
There was a problem hiding this comment.
I think this needs to first check that: before you load the builtin, it's not in the moduleLoad list. Afterwards, it is. And the difference of whether import defer is implemented is whether it's there right away or only after you have accessed anything from http.
There was a problem hiding this comment.
@joyeecheung The thing is, it's already in the module load list, even before accessing the module. E.g. this code
let modules = process.moduleLoadList.filter((item) => item.endsWith('http'))
console.log(modules);
prints [ 'NativeModule internal/http', 'NativeModule http' ]. Do you know why it's loaded eagerly?
There was a problem hiding this comment.
You mean in the test or standalone? If you run this snippet alone it's pretty both on main branch and in current releases. If you have code that imports http then it surely will print that because ESM import is hoisted and dependencies are loaded before the current module is executed, even if you put the import statement below the log.
The tests added ensure that Node.js doesn't crash or produce incorrect results when importing synthetic modules (i.e. JSON, text or builtin modules) with the `defer` modifier. Signed-off-by: Maya Lekova <maya@igalia.com>
c6bb77a to
46df383
Compare
|
|
||
| // Test that uses import.defer for a builtin module. Currently | ||
| // defer importing of a synthetic module should be a no-op | ||
| // in Node.js, so the test is mostly a smoke test that Node |
There was a problem hiding this comment.
| // in Node.js, so the test is mostly a smoke test that Node | |
| // in Node.js as they are born pre-evaluated, so the test is mostly a smoke test that Node |
| with { type: 'json' }; | ||
|
|
||
| // Check that the imported object has the expected key/value. | ||
| assert.strictEqual(imported_json.default.rocko, 'artischocko'); |
There was a problem hiding this comment.
Can we also test that imported_json.default === same_import_but_without_defer.default?
test: add smoke tests for defer importing synthetic modules
The tests added ensure that Node.js doesn't crash or produce incorrect results when importing synthetic modules (i.e. JSON, text or builtin modules) with the
defermodifier.Signed-off-by: Maya Lekova maya@igalia.com