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,tools: move Flags: comment processing from Python to JavaScript #25167

Closed
wants to merge 40 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 21, 2018

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

@Trott Trott added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Dec 21, 2018
@addaleax
Copy link
Member

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
Copy link
Member Author

Trott commented Dec 21, 2018

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
Copy link
Member Author

Trott commented Dec 21, 2018

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/

doc/guides/writing-tests.md Outdated Show resolved Hide resolved
doc/guides/writing-tests.md Outdated Show resolved Hide resolved
doc/guides/writing-tests.md Outdated Show resolved Hide resolved
@Trott Trott force-pushed the flags branch 2 times, most recently from c8db3b7 to 6d4ecb6 Compare December 21, 2018 21:18
@Trott
Copy link
Member Author

Trott commented Dec 21, 2018

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

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

@Trott
Copy link
Member Author

Trott commented Dec 22, 2018

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

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

@richardlau
Copy link
Member

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
Copy link
Member Author

Trott commented Dec 22, 2018

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 Trott force-pushed the flags branch 3 times, most recently from 4bac563 to 7a7fd9d Compare December 27, 2018 16:34
@Trott
Copy link
Member Author

Trott commented Dec 27, 2018

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

@richardlau Done!

@Trott
Copy link
Member Author

Trott commented Dec 27, 2018

@Trott
Copy link
Member Author

Trott commented Dec 27, 2018

Copy link
Member

@BridgeAR BridgeAR left a 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.

test/common/index.js Outdated Show resolved Hide resolved
test/common/index.js Show resolved Hide resolved
test/common/index.js Outdated Show resolved Hide resolved
test/common/index.js Show resolved Hide resolved
test/parallel/test-buffer-of-no-deprecation.js Outdated Show resolved Hide resolved
test/sequential/test-inspector-overwrite-config.js Outdated Show resolved Hide resolved
test/common/index.js Outdated Show resolved Hide resolved
test/es-module/test-esm-loader-invalid-format.mjs Outdated Show resolved Hide resolved
@devsnek
Copy link
Member

devsnek commented Dec 27, 2018

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
Copy link
Member Author

Trott commented Dec 27, 2018

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
Copy link
Member

@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').

@Trott Trott force-pushed the flags branch 3 times, most recently from 5c6a4c5 to 4381c4d Compare December 31, 2018 03:37
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().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants