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 runner: recursive or globbed loading of non-JS test files #44023

Closed
meyfa opened this issue Jul 28, 2022 · 52 comments · Fixed by #44241
Closed

Test runner: recursive or globbed loading of non-JS test files #44023

meyfa opened this issue Jul 28, 2022 · 52 comments · Fixed by #44241
Labels
feature request Issues that request new features to be added to Node.js. loaders Issues and PRs related to ES module loaders test_runner Issues and PRs related to the test runner subsystem.

Comments

@meyfa
Copy link
Contributor

meyfa commented Jul 28, 2022

What is the problem this feature will solve?

As per https://nodejs.org/dist/latest-v18.x/docs/api/test.html#test-runner-execution-model, the test runner can be started as node --test dirname/ to recursively run all test files (ending in .js, .cjs or .mjs) contained in the directory dirname. This unfortunately means that non-JS test files, specifically test files written in TypeScript in my case, will not be found since they end in .ts instead of .js. The docs say to provide each test file separately, e.g. node --loader=ts-node/esm --test dirname/foo.test.ts dirname/bar.test.ts but this gets messy very quickly.

What is the feature you are proposing to solve the problem?

I see a few options (in order of my preference, starting with most preferred):

  • Support globbing (e.g., node --loader ts-node/esm --test "dirname/**/*.ts"). Note that this does work on Linux, to some extent, since the shell will expand asterisks. This doesn't work on Windows, though, and doesn't work when the string is quoted. Tools like mocha will perform globbing independently of the operating system or shell.
  • Provide a CLI arg for additional test file extensions (e.g., node --loader ts-node/esm --test --test-extensions=.ts,.cts,.mts dirname/).
  • Have some sort of configuration file for the test runner where this can be configured.
  • Let loaders tell Node about additional file extensions (e.g., node --loader ts-node/esm --test dirname/ and ts-node could hook into the directory traversal).

What alternatives have you considered?

  • Listing test files manually (hard to maintain)
  • Having a script do the globbing and construct the command line with all files explicitly listed
  • Running node --test pointed to an index file that performs the globbing and dynamically imports all actual test files (very ugly; loses process separation between test files)

I also opened an issue at ts-node (TypeStrong/ts-node#1853) specifically for TypeScript, to see if they are interested in better test runner support.

@meyfa meyfa added the feature request Issues that request new features to be added to Node.js. label Jul 28, 2022
@benjamingr
Copy link
Member

@nodejs/test_runner

@benjamingr
Copy link
Member

Honestly, I would just add .ts support explicitly (and probably .tsx/.jsx) given how common it is compared to the alternative with a helpful message in case people didn't pass a loader/require hook.

@MoLow MoLow added the test_runner Issues and PRs related to the test runner subsystem. label Jul 28, 2022
@MoLow
Copy link
Member

MoLow commented Jul 28, 2022

probably for starters, as @benjamingr support /.tsx?$/ as well,
eventually allow configuration of custom file filtering

@ljharb
Copy link
Member

ljharb commented Jul 29, 2022

I don’t think supporting ts (or jsx) by default makes any sense if TS/JSX content isn’t auto loaded.

@MoLow
Copy link
Member

MoLow commented Jul 29, 2022

Perhaps we should implement a solution for #43895 (that will allow passing a custom function to filter files)

I am willing to implement some --test-config flag if there is consensus on it.
CC @nodejs/test_runner

@aduh95
Copy link
Contributor

aduh95 commented Jul 29, 2022

Maybe a better solution would be to have an option for the loader to have an option to tell which file patten it supports for tests? /cc @nodejs/loaders

@MoLow
Copy link
Member

MoLow commented Jul 29, 2022

Maybe a better solution would be to have an option for the loader to have an option to tell which file patten it supports for tests?

why will it be better? I think a configuration file will cover more use-cases (real usecases from here), for example:

  • within a file, filter which tests should run
  • support glob (without relation to ts support)

loaders should process test files regardless of this discussion, there is a seperate issue about that

@aduh95
Copy link
Contributor

aduh95 commented Jul 29, 2022

Running the test runner on a .ts file only makes sense if there's a loader able to understand it, so it's convenient if the loader itself can provide this info. If the info comes from somewhere else, it's more likely to de-sync at some point. Anyway that was just a thought.

@ljharb
Copy link
Member

ljharb commented Jul 29, 2022

I completely agree that that’s the only way to handle it. Nothing should be implicitly included unless node understands it;cams adding a loader should also implicitly include anything the loader handles. Users should only have to configure file extensions if they’re deviating from their loaders’ defaults.

@MoLow
Copy link
Member

MoLow commented Jul 29, 2022

I might not fully understand how loaders work, so correct me in case I am wrong - a loader does not expose what file extensions it supports,
So there is no way for --test to know what to extensions to filter
If we are talking about a new flag/option declaring what extensions a loader supports in the context of running tests - I don't really see a reason that should be part of loaders and not just part of the test runner config?

@meyfa
Copy link
Contributor Author

meyfa commented Jul 29, 2022

Personally, I see a huge advantage in letting users specify the file pattern precisely: I often find myself putting test fixtures, shared code etc. in the test/ directory (e.g. test/fixtures.ts), along with test files (such as test/foo.test.ts, test/bar.test.ts). In these cases, I would want to use a glob like test/**/*.test.ts instead of basing it off of the .ts extension.

@cspotcode
Copy link

I see mts and cts file extensions were not mentioned above. This is evidence in favor of allowing users and customization hooks to specify.

@benjamingr
Copy link
Member

I think probably just allowing an RE or glob of test files then as an argument for --test ?

@MoLow
Copy link
Member

MoLow commented Jul 30, 2022

I think probably just allowing an RE or glob of test files then as an argument for --test ?

core doesn't currently support glob and regex can be hard to distinguish from paths

@aduh95
Copy link
Contributor

aduh95 commented Jul 30, 2022

I think probably just allowing an RE or glob of test files then as an argument for --test ?

core doesn't currently support glob and regex can be hard to distinguish from paths

Currently --test doesn't take any value right? So we could introduce a semver-minor change saying that now --test can take an optional string value that will be interpreted as a regex I think.

@MoLow
Copy link
Member

MoLow commented Jul 30, 2022

Currently --test doesn't take any value right? So we could introduce a semver-minor change saying that now --test can take an optional string value that will be interpreted as a regex I think.

it currently accepts paths

@aduh95
Copy link
Contributor

aduh95 commented Jul 30, 2022

Currently --test doesn't take any value right? So we could introduce a semver-minor change saying that now --test can take an optional string value that will be interpreted as a regex I think.

it currently accepts paths

That's incorrect, --test is a boolean flag:

bool test_runner = false;

node/src/node_options.cc

Lines 529 to 531 in d29e78a

AddOption("--test",
"launch test runner on startup",
&EnvironmentOptions::test_runner);

We could potentially change it to also accept a string. (i.e. node --test='/.\.test\.ts$/' path/to/test/dir)

@MoLow
Copy link
Member

MoLow commented Jul 30, 2022

@aduh95 see

const args = ['--test', 'a-random-file-that-does-not-exist.js'];

const args = ['--test', testFixtures];

const args = ['--test', testFixtures, join(testFixtures, 'index.js')];

https://github.com/nodejs/node/blob/2fe4e9473fb50630326754886dee1d301a1a661e/test/parallel/test-runner-exit-code.js#L47
etc

@JakobJingleheimer
Copy link
Contributor

I think a glob would an ideal solution:

  • It is very common in test runners (I can't think of any test runner that does not support it), so users will already be familiar
  • It addresses several other issues, such as using spec instead of test (eg foo.spec.js)

If node does not understand the file it was explicitly pointed to, user-error. I can't imagine a user suddenly complaining that node didn't support typescript before and still doesn't when fed a glob on typescript. I think this will be even less of an issue when we add the more detailed error message to ESMLoader::resolve() when an unknown file is encountered.

@MoLow
Copy link
Member

MoLow commented Jul 31, 2022

assuming that adding glob to core has a bigger scope than a small fix to the test runner,
what will be the process of adding this to core? which teams need to be CC'd and consulted before working on it?
and should some collaborator/tsc member contact user-land glob pacakge mainaners as a lesson learned from #43382?

@JakobJingleheimer
Copy link
Contributor

Perhaps node:fs?

import { glob } from 'node:fs/promises';

const [
  file1,
  file2,
  ...files
] = await glob('./src/**/*.spec.mjs');

As for the team, I imagine it depends on where it lives.

Yes, probably a good idea to reach out for lessons learnt 🙂

@MoLow
Copy link
Member

MoLow commented Jul 31, 2022

I see that this was already raised here #40731 (comment) and #40954 (comment) with no real objections
@isaacs can you update where this stands? there is now a real use case for using minimach internally,
I am willing to help with porting/implementing if help is needed and ok with you

@cspotcode
Copy link

Something else to consider: will node support a single flavor of globbing, or will it support multiple behaviors via options, similar to bash's various shopts: dotglob, extglob, etc? I'm guessing node --test <glob> is interpreted in one and only one way, but the {glob} from 'fs' API will support options?

@GeoffreyBooth
Copy link
Member

I’m growing concerned by the number of dependencies that are becoming part of core as part of shipping our own test runner. Like are we going to vendor https://github.com/isaacs/node-glob or https://github.com/isaacs/minimatch now too, or create our own slimmed-down versions or alternatives? This feels a lot like the path we went down with chalk in #43382.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 1, 2022

I’m growing concerned by the number of dependencies that are becoming part of core as part of shipping our own test runner.

What dependencies has core already taken on because of the test runner?

Like are we going to vendor https://github.com/isaacs/node-glob or https://github.com/isaacs/minimatch now too, or create our own slimmed-down versions or alternatives?

It looks like that may already be in the works independent of the test runner. If they do make it into core, there is no reason for the test runner not to use them.

@cspotcode
Copy link

We should also consider if an equivalent to this pseudo-code makes sense: require('node:test').runDashDashTest(['pathA', 'pathB'])

Equivalent to the behavior of the --test flag. Understanding that yeah, --test spawns child processes already. , but it reduces the total number by one.

@benjamingr
Copy link
Member

@cspotcode yes that's a suggestion in #44023 (comment) :)

Also making sure you don't miss the meeting doodle at #44023 (comment)

@aduh95
Copy link
Contributor

aduh95 commented Aug 4, 2022

Also, @aduh95 can you please add me (and @cjihrig probably since he wrote the thing?) to the @nodejs/test_runner team?

That's done!

I (personally) think that glob is common enough that users expect it to be part of core like regular expressions or dates like it is in Python/C#/other runtimes.

There have been discussions to include it in fs (see #40731 (comment) and #40731 (comment)), so IMO a better API would be:

require('node:test').run({
  concurrency: true,
  timeout: 10_000, // whatever
  files: require('node:fs').globSync("**/*.test.ts"),
});

IMO there should be a way to a file which name would be*.test.ts in a directory named ** without the whole string being interpreted as a glob – of course that's a silly example, but I really think there's virtue in keeping such transforms explicit, same as we require file: URLs to be wrapped in new URL().

Other than that, I think that's a very neat idea, I like both options.

@MoLow
Copy link
Member

MoLow commented Aug 5, 2022

I am +1 on import { runTests } from 'node:test'

However, import { config } from 'node:test' doesn't sound correct:
What happens if two different configurations are set in two different files?
What happens if it is configured once using --import and then overridden inside some of the test files?

@aduh95
Copy link
Contributor

aduh95 commented Aug 5, 2022

What happens if two different configurations are set in two different files?
What happens if it is configured once using --import and then overridden inside some of the test files?

I assume config would only apply when --test is passed, so it would only have an effect if it's pre-imported (using --require or --import).

@MoLow
Copy link
Member

MoLow commented Aug 5, 2022

I assume config would only apply when --test is passed, so it would only have an effect if it's pre-imported (using --require or --import).

that is pretty much what I suggested, but instead of using --test-config it will use --import?

@benjamingr
Copy link
Member

One more ping to @nodejs/test_runner @cjihrig @aduh95 for the meeting doodle to make sure you saw it https://doodle.com/meeting/participate/id/dwjGRJmb

@GeoffreyBooth
Copy link
Member

However, import { config } from 'node:test' doesn't sound correct:
What happens if two different configurations are set in two different files?

I feel like we should solve the overall question of how to configure Node from a config file, before solving specific use cases like node:test and loaders. See also #43973.

@MoLow
Copy link
Member

MoLow commented Aug 9, 2022

@benjamingr is there a verdict for the meeting time?

@benjamingr
Copy link
Member

Hey, due to a combination of bereavement leave and food poisoning / tummy flu I've totally dropped the ball on this and won't be able to engage in the next 3-4 days (at least until the Shiv'ah is after us).

I'll send another doodle next week (hopefully) or if someone else wants to pick it up - go tor it.

@MoLow MoLow closed this as completed in 59527de Aug 24, 2022
RafaelGSS pushed a commit that referenced this issue Sep 5, 2022
PR-URL: #44241
Fixes: #44023
Fixes: #43675
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Fyko pushed a commit to Fyko/node that referenced this issue Sep 15, 2022
PR-URL: nodejs#44241
Fixes: nodejs#44023
Fixes: nodejs#43675
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
MoLow added a commit to MoLow/node that referenced this issue Oct 3, 2022
PR-URL: nodejs#44241
Backport-PR-URL: nodejs#TBD
Fixes: nodejs#44023
Fixes: nodejs#43675
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
MoLow added a commit to MoLow/node that referenced this issue Oct 3, 2022
PR-URL: nodejs#44241
Backport-PR-URL: nodejs#44873
Fixes: nodejs#44023
Fixes: nodejs#43675
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
MoLow added a commit to MoLow/node that referenced this issue Oct 3, 2022
PR-URL: nodejs#44241
Backport-PR-URL: nodejs#44873
Fixes: nodejs#44023
Fixes: nodejs#43675
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
MoLow added a commit to MoLow/node that referenced this issue Oct 3, 2022
PR-URL: nodejs#44241
Backport-PR-URL: nodejs#44873
Fixes: nodejs#44023
Fixes: nodejs#43675
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
MoLow added a commit to MoLow/node that referenced this issue Oct 12, 2022
PR-URL: nodejs#44241
Backport-PR-URL: nodejs#44873
Fixes: nodejs#44023
Fixes: nodejs#43675
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
MoLow added a commit to MoLow/node that referenced this issue Oct 12, 2022
PR-URL: nodejs#44241
Backport-PR-URL: nodejs#44873
Fixes: nodejs#44023
Fixes: nodejs#43675
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
MoLow added a commit to MoLow/node that referenced this issue Nov 23, 2022
PR-URL: nodejs#44241
Backport-PR-URL: nodejs#44873
Fixes: nodejs#44023
Fixes: nodejs#43675
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
richardlau pushed a commit that referenced this issue Dec 7, 2022
PR-URL: #44241
Backport-PR-URL: #44873
Fixes: #44023
Fixes: #43675
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this issue Jan 3, 2023
PR-URL: nodejs/node#44241
Backport-PR-URL: nodejs/node#44873
Fixes: nodejs/node#44023
Fixes: nodejs/node#43675
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this issue Jan 3, 2023
PR-URL: nodejs/node#44241
Backport-PR-URL: nodejs/node#44873
Fixes: nodejs/node#44023
Fixes: nodejs/node#43675
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
MoLow added a commit to MoLow/node-core-test that referenced this issue Feb 2, 2023
PR-URL: nodejs/node#44241
Fixes: nodejs/node#44023
Fixes: nodejs/node#43675
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
(cherry picked from commit 59527de13d39327eb3dfa8dedc92241eb40066d5)
MoLow added a commit to MoLow/node-core-test that referenced this issue Feb 2, 2023
PR-URL: nodejs/node#44241
Fixes: nodejs/node#44023
Fixes: nodejs/node#43675
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
(cherry picked from commit 59527de13d39327eb3dfa8dedc92241eb40066d5)
MoLow added a commit to MoLow/node-core-test that referenced this issue Feb 2, 2023
PR-URL: nodejs/node#44241
Fixes: nodejs/node#44023
Fixes: nodejs/node#43675
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
(cherry picked from commit 59527de13d39327eb3dfa8dedc92241eb40066d5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. loaders Issues and PRs related to ES module loaders test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants