nodejs / admin

Administrative space for policies of the TSC
158 stars 135 forks source link

Deprecate `test` npm package #928

Open RedYetiDev opened 1 month ago

RedYetiDev commented 1 month ago

The test npm package serves as a backport of the node:test module for Node.js versions prior to v18. However, all these older versions are now end-of-life (EoL). This means the package offers no real advantage for users running supported (non-EoL) versions of Node.js, which they ideally should be using.

Given that the package is outdated and no longer necessary, wouldn't it make sense to deprecate it?

CC @nodejs/test_runner

cjihrig commented 1 month ago

Alternative take: it would be great to bring that module up to date. The delta between node:test on v18 vs. v23 is substantial.

RedYetiDev commented 1 month ago

Alternative take: it would be great to bring that module up to date. The delta between node:test on v18 vs. v23 is substantial.

That could be done, I just feel that there should be a point where this module reaches the end of its lifespan. IMHO it shouldn't be a polyfill/shim forever.

Maybe it's okay now that the test runner was added recently, but in a few years, maybe this module should eventually be deprecated.

RedYetiDev commented 1 month ago

I've looked into upgrading the package, and I'm afraid it's not that simple. The test runner now makes use of internalBinding (which can fairly easily be polyfilled to produce the same result), and internal/modules/esm/loader, which is less straightforward to polyfill.

cjihrig commented 1 month ago

internal/modules/esm/loader, which is less straightforward to polyfill.

In userland code, it might be enough to use dynamic import.

juliangruber commented 1 month ago

Last time I tried upgrading the test package I gave up because I ran out of time & energy

aduh95 commented 1 month ago

Yeah because that package is using a different set of lint rules, there are lots of conflicts to fix every time we try to backport something. At this point, we should probably disable the linter on files maintained on nodejs/node. (or indeed, deprecate the package)

RedYetiDev commented 1 month ago

I also tried upgrading, but there are so many things that are extremely complicated to include.

(My progress: 34/35 passing tests) The following features are currently unsupported:

RedYetiDev commented 1 month ago

Okay. I've upgraded the package to v23, but with the amount of work it took, and the likelyhood of it being out-of-date again in only a few months, I still think deprecation is the way to go.

bnb commented 2 weeks ago

hey! Just ran into this with one of my older packages.

I'd strongly urge us to keep this going, as it does allow for the option to decouple your tests from your runtime - having to do test and runtime upgrades at the same time does add burden, and being able to swap over to test or just always use test really smooths out that pain point.

Further, generally people update their runtime less frequently than they update their dependencies. Having the separate package enables people to get newer features from the test runner "sooner", which will help adoption of the tooling generally.

bnb commented 2 weeks ago

I am happy to dedicate time to this where I'd be helpful, but I'm not actually sure if I the technical context for the native bits of the test suite as it exists now.

RedYetiDev commented 2 weeks ago

See https://github.com/nodejs/node-core-test/pull/54 for the upgrade, if you'd like to dedicate time, reviews would be nice :heart: