test,tools: move Flags: comment processing from Python to JavaScript

[Trott]

This is a big change set but one that can be broken down into many smaller pull requests for easier review if necessary.

This adds common.relaunchWithFlags() along with two useful-but-not-strictly-necessary helper functions, common.exposeInternals() and common.experimentalWorker().

This allows tests to relaunch themselves with the appropriate command line flags.

Advantages:

• Tests work right from the command line without having to specify the flags or use the Python test runner.
• Less magic. Having what is going on be presented explicitly in the executable JavaScript is preferable to comments affecting code but only affecting it when run a certain way.
• Related to the previous two, but this is more friendly to new contributors and sporadic/infrequent contributors.
• This removes a small bit of dependency on Python that isn't necessary. Arguably, this aligns with Suggestion: New Python & GYP strategic initiative or working group TSC#642.

Downside:

• It takes slightly longer for these tests to run. Not much, though. On my laptop, make test with this changeset took 5 minutes and 23 seconds. On master, it took 5 minutes and 16 seconds.

Upside to the downside:

• If this encourages us to go through and figure out places where tests can be appropriately and correctly refactored to not use CLI flags, that's a win. In many cases, the use of a flag (especially --expose-internals) could be a sign the test is testing the wrong thing.

@nodejs/testing

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

@Trott build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2027/pipeline

[addaleax]

Less magic.

I think this is a bit more magic, tbh?

But more concretely, I think this might interfere with some of the “advanced” modes we have in the Python test runner, at least --valgrind and --worker?

[Trott]

I think this is a bit more magic, tbh?

I can certainly agree that there's a real danger it will be more brittle. If so, that's a good reason not to do it. I see it already messes up one test that escaped my notice by leaving around some stale processes.

[Trott]

Fixed the one test that was leaving around stale processes.

Let's try a full CI and see what breaks...

CI: https://ci.nodejs.org/job/node-test-pull-request/19724/

[Trott]

Fixed test-module-cjs-helpers which was failing with --worker.

CI: https://ci.nodejs.org/job/node-test-pull-request/19729/

[Trott]

Fixed es-modules so they pass when tested in workers.

CI: https://ci.nodejs.org/job/node-test-pull-request/19736/ ✔️

[richardlau]

This adds common.relaunchWithFlags() along with two useful-but-not-strictly-necessary helper functions, common.exposeInternals() and common.experimentalWorker().

If we do this, I think I'd prefer a common.requiresFlags() that checks and relaunches if necessary. With the proposed common.relaunchWithFlags() the tests are having to add more logic to conditionally call it.

[Trott]

If we do this, I think I'd prefer a common.requiresFlags() that checks and relaunches if necessary. With the proposed common.relaunchWithFlags() the tests are having to add more logic to conditionally call it.

I did it this way to keep the flexibility of doing feature detection (e.g., checking for global.gc rather than --expose-gc) but I think you're probably right. The benefit is small and the cost significant.

[Trott]

If we do this, I think I'd prefer a common.requiresFlags() that checks and relaunches if necessary.

@richardlau Done!

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/19844/

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/19845/ ✔️

[BridgeAR]

I planned on doing something very similar without changing the flags but I am fine with the proposed explicit way as well.

I just do not like the changes to the es-module tests at all. Those seem very verbose now and I suggest to keep the old detection in there for now until we figure out a better way to deal with those.
It might be possible to fix those easily, in that case this is LGTM with my comments addressed.

[devsnek]

Honestly... i'm not a fan. I think for the purposes of tests, a nice comment syntax is way easier for both the people writing them and for the runner.

[Trott]

Honestly... i'm not a fan. I think for the purposes of tests, a nice comment syntax is way easier for both the people writing them and for the runner.

Yeah, I'm not much of a fan myself, and I wrote the thing.

I'm going to keep pushing this boulder uphill a bit more to see if there's stuff worth salvaging in here somewhere. If it required modifying 40 test files, I'd be OK with it. But 400? 😱

The one thing that makes me reluctant to just drop it entirely right away is the (perhaps incorrect) belief that we will want to get away from the Python test runner at some point. Not sure I can really defend that belief with logic, though.

Ruben took the time to review it, so I should at least make those changes and see what kind of state it's in after that!

[richardlau]

@Trott Thanks for pushing this boulder! I apologise that I probably won't be able to review the technical aspects of this PR this week (so don't let me hold this up, but I don't think there's a rush for this either). I'm not sure where I stand conceptually on moving away from the Python test runner (part of the appeal of the current set up is that in theory the runner and the thing running the runner are stable and not affected by a recompilation of Node.js).

One thing possibly in favour of this PR is that I think a combination of this PR (or alternatively a revert of #24876) and #25256 would remove the requirement for all tests to require('../common').