Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add test for experimental ESM modules #74

Merged
merged 3 commits into from
Apr 12, 2019
Merged

test: add test for experimental ESM modules #74

merged 3 commits into from
Apr 12, 2019

Conversation

bcoe
Copy link
Owner

@bcoe bcoe commented Apr 7, 2019

Adds test demonstrating using c8 for collecting coverage for experimental modules (see conversation here).

I've been meaning to add this test for a while, but haven't had many OSS cycles... it's cool to see it work after months of noodling on V8/Node.js.

Background

The new module system in Node.js does not support require.extensions; while there is some work in motion to support similar extension mechanisms, it's unlikely to exist in time for --experimental-modules to be un-flagged.

c8 collects coverage using the inspector and the NODE_V8_COVERAGE environment variable, this is built into Node.js itself and does not require extending loaders.

Checklist
  • npm test, tests passing
  • npm run test:snap (to update the snapshot)
  • tests and/or benchmarks are included

CC: @MylesBorins, @joyeecheung, @guybedford, @GeoffreyBooth, @coreyfarrell (CC: party 🎉 woo).

@coreyfarrell
Copy link

I tested this with https://github.com/coreyfarrell/c8-test, worked under node.js 11.13.0 but not 10.15.3. Is c8 with experimental-modules under node.js 10 not possible?

The test repo does equivalent testing with nyc using the esm module for easy comparison, the results under node.js 11 seem reasonable for both.

@bcoe
Copy link
Owner Author

bcoe commented Apr 7, 2019

@coreyfarrell it should work under 10, but you'd need to play around with wrapper-length I believe, might need to fiddle with a couple other things.

@coreyfarrell
Copy link

@bcoe we recently saw a change in node.js 11.11.0 which switched require from vm.runInThisContext to vm.compileFunction, is this the reason the needing to fiddle with wrapper-length?

@coreyfarrell
Copy link

Odd, if I set "wrapper-length": 0 in the config for c8 it works for node.js 10 and 11.

@bcoe
Copy link
Owner Author

bcoe commented Apr 8, 2019

@coreyfarrell the Node@10 issues should be addressed by the following:

nodejs/node@23ea7ee

@bcoe bcoe merged commit 1dabece into master Apr 12, 2019
@bcoe bcoe deleted the esm branch April 12, 2019 22:23
mcknasty pushed a commit to mcknasty/c8 that referenced this pull request Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants