Clean up the test harness a bit #55
Conversation
test/version-check.js
Outdated
| // Do not want this running on loader thread | ||
| if (wt.isMainThread) { |
There was a problem hiding this comment.
Style nit: I would keep the success case from being nested:
| // Do not want this running on loader thread | |
| if (wt.isMainThread) { | |
| // Do not want this running on loader thread | |
| if (wt.isMainThread === false) return |
There was a problem hiding this comment.
Ah, I have muscle memory around linters that don't understand that CJS files are actually function blocks, so generally avoid top-level returns, even if they're warranted. Since the linter here allows it, I'll use it. Update incoming.
There was a problem hiding this comment.
I guess I've been ignoring such lint errors or just haven't seen them 😅
Primarily, this gets rid of the `test/runtest` executable, preventing the need for spawning an additional subprocess for each test. The current approach handles all the things that `test/runtest` would in a `--require` and in a loader that switches whether or not to use the IITM loader via the same filename check that was originally done in `test/runtest`. One interesting thing that happened while making this change is that we need some way of passing the entrypoint filename to the loader. Since newer versions of Node.js run the loader in a separate thread, `process.argv[1]` no longer works. The approved alternative is to use inter-thread messages via the `register()` method, but that's not available on some versions of Node.js that also use loader threads. To get around this, an environment variable is set in the `--require` that's then propagated to the loader thread. This is a bit hacky, but probably the only thing that will work across the supported version range. An attempt was made to use the more popular `tap` test package, rather than `imhotap`, but there's no version of `tap` that supports both the entire Node.js version range that IITM supports _and_ also supports ESM. For now, this means sticking with `imhotap`, but were it not for the incompatibility, `tap` would be just as suitable here, and is a much more popular test package.
bengl
force-pushed
the
bengl/test-harness-cleanup
branch
from
bf2a083 to
68abc1d
Compare
The wrapper loader used for testing is just a pass-through. It's not chaining loaders or anything like that. It should be completely transparent. What's the concern? |
It's not a blocker. I'm just unclear what a debugging session will be like. |
|
@jsumners-nr You make a good point here that the test README should be updated. I'll do that now. |
Primarily, this gets rid of the
test/runtestexecutable, preventing the need for spawning an additional subprocess for each test. The current approach handles all the things thattest/runtestwould in a--requireand in a loader that switches whether or not to use the IITM loader via the same filename check that was originally done intest/runtest.One interesting thing that happened while making this change is that we need some way of passing the entrypoint filename to the loader. Since newer versions of Node.js run the loader in a separate thread,
process.argv[1]no longer works. The approved alternative is to use inter-thread messages via theregister()method, but that's not available on some versions of Node.js that also use loader threads. To get around this, an environment variable is set in the--requirethat's then propagated to the loader thread. This is a bit hacky, but probably the only thing that will work across the supported version range.An attempt was made to use the more popular
taptest package, rather thanimhotap, but there's no version oftapthat supports both the entire Node.js version range that IITM supports and also supports ESM. For now, this means sticking withimhotap, but were it not for the incompatibility,tapwould be just as suitable here, and is a much more popular test package.