-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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,tools: move Flags: comment processing from Python to JavaScript #25167
Conversation
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 |
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. |
Fixed the one test that was leaving around stale processes. Let's try a full CI and see what breaks... |
c8db3b7
to
6d4ecb6
Compare
Fixed test-module-cjs-helpers which was failing with |
Fixed es-modules so they pass when tested in workers. CI: https://ci.nodejs.org/job/node-test-pull-request/19736/ ✔️ |
If we do this, I think I'd prefer a |
I did it this way to keep the flexibility of doing feature detection (e.g., checking for |
4bac563
to
7a7fd9d
Compare
@richardlau Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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! |
@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 |
5c6a4c5
to
4381c4d
Compare
The --no-deprecation flag specified in test-module-children suppresses a deprecation warning that does not affect test results. Remove it.
Use common.relaunchWithFlags() for --require. An incidental advantage is this allows us to enable the inspector check for this test.
Flags can now be handled in code directly, so no need to check and warn the user if there is a stray Flags: comment.
Remove the processing of `Flags:` comments and associated documentation. This is now handled by `common.relaunchWithFlags()`.
Replace common.exposeInternals() with common.requireFlags(['--expose-internals']).
Move special logic for common.experimentalWorker() into common.requireFlags(). Replace all calls to common.experimentalWorker() with calls to common.requireFlags().
Replace all instances of common.relaunchWithFlags() with common.requireFlags().
common.requireFlags() does some checking before relaunching. Some of those checks are duplicated in the tests calling common.requireFlags(). Remove instances of duplicate logic.
Use `...flags` instead of `[flags]` for the common.requireFlags().
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()
andcommon.experimentalWorker()
.This allows tests to relaunch themselves with the appropriate command line flags.
Advantages:
Downside:
make test
with this changeset took 5 minutes and 23 seconds. On master, it took 5 minutes and 16 seconds.Upside to the downside:
--expose-internals
) could be a sign the test is testing the wrong thing.@nodejs/testing
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes