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

Add test runner configuration #48852

Closed
remcohaszing opened this issue Jul 20, 2023 · 9 comments
Closed

Add test runner configuration #48852

remcohaszing opened this issue Jul 20, 2023 · 9 comments
Labels
feature request Issues that request new features to be added to Node.js. stale

Comments

@remcohaszing
Copy link

remcohaszing commented Jul 20, 2023

What is the problem this feature will solve?

Currently the test runner can be configured using command line arguments and / or a custom script that calls run(). These commands can get long, making them hard to read. For example:

{
  "scripts": {
    "test": "c8 node --test --test-reporter @reporters/junit --test-reporter-destination=junit.xml --test-reporter spec --test-reporter-destination stdout"
  }
}

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

I propose to add the key test to package.json. Running node --test will read this key and merge the configuration with other configuration options. The test key accepts the same options as run(), plus the reporters field, which is a key-value pair of reporters and their output file.

The above example, could be changed to:

{
  "scripts": {
    "test": "c8 node --test"
  },
  "test": {
    "reporters": {
      "@reporters/junit": "junit.xml",
      "spec": "stdout"
    }
  }
}

This makes it nicer to work with TypeScript interpreters. (Refs privatenumber/tsx#257.) For example:

{
  "scripts": {
    "test": "c8 tsx --test"
  },
  "test": {
    "testNamePatterns": "test/*.ts"
    "reporters": {
      "@reporters/junit": "junit.xml",
      "spec": "stdout"
    }
  }
}

The test field can also be defined using a JSON schema (outside of Node.js), so editors get validation and completion.

What alternatives have you considered?

It could be defined in a separate file. I think a key in package.json makes sense though, because it’s already used to configure Node.js.

@remcohaszing remcohaszing added the feature request Issues that request new features to be added to Node.js. label Jul 20, 2023
@MoLow
Copy link
Member

MoLow commented Jul 20, 2023

this was raised before some people had concerns, but I am definitely +1 on this @nodejs/test_runner WDYT?

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Jul 20, 2023

This came up recently in #48425 (comment). See also https://github.com/orgs/nodejs/discussions/44975 and #43973 (the latter is very similar to this feature request).

I would love to see Node be configurable via package.json or some other config file, but I think such configuration needs to cover all of Node’s configuration1, not just a subset like the test runner or loaders. This is tricky because a lot of Node’s configuration gets used during startup by C++ code, so any implementation needs to read and parse the config file before the first config value gets used. In the discussion elsewhere we think that the easiest way to achieve this would be to give Node support for .env files like Bun already has, as this would let people define Node’s config as a NODE_OPTIONS variable within such a file, as well as any other environment variables an app needs (as in, it solves not only the “configure Node” use case but also the “configure app” use case). It’s probably also easier to write C++ to parse a .env file than a JSON file. We can also always add support for configuration from package.json or elsewhere in addition to the .env file, just as Bun supports multiple config files.

Footnotes

  1. Or realistically, all of Node’s configuration supported by the NODE_OPTIONS environment variable. There are a handful of command line flags that are unsupported there, that presumably would be unsupported by a config file as well.

@benjamingr
Copy link
Member

I'm +0.5, but can someone make the case of what's the difference between having a config file that also calls run and just having a config file? Like, other than "that's what other runners do" what's the rationale?

@remcohaszing
Copy link
Author

It makes sense to me to support more options than just those for the test runner.

I am not familiar with Node internals, so if parsing JSON is hard at the level it’s needed, I’m inclined to believe whatever you tell me.

I am personally just not a fan of .env files though. They can only contain string key-value pairs, but sometimes another type makes more sense. For example, for to map reporters to the output files, it makes sense to use a map / object. I saw https://github.com/orgs/nodejs/discussions/44975#discussioncomment-3881320, which proposes to add more environment variables. I think this makes it better, but still, this looks weird to me:

NODE_TEST_REPORTER='@reporters/junit spec'
NODE_TEST_REPORTER_DESTINATION='junit.xml stdout'

what's the difference between having a config file that also calls run and just having a config file?

One useful feature of node --test, is that test files can also be explicitly passed as a command line argument. Having a configuration, it could be possible to run node path/to/specific.test.js, and still have the configuration applied. Also run() currently doesn’t accept reporters.

Also if such a configuration is standardized, third party integrations such as editor integrations could read this configuration. If we take the configuration a bit further, it could look like this:

{
  "scripts": {
    "test": "c8 node --test"
  },
  "test": {
    "loaders": ["tsx"],
    "testNamePatterns": "test/*.ts"
    "reporters": {
      "@reporters/junit": "junit.xml",
      "spec": "stdout"
    }
  }
}

This would tell all integrations to use the tsx loader.

@koshic
Copy link

koshic commented Jul 22, 2023

Strongly -1 for extending package.json or another kind of 'hardcoded' config. My own 'test-runner' implementation is about 100 lines for all: test.config.xxx (js, mjs or ts) parsing, setup ESM loaders, collecting coverage via c8 Report class, choosing right reporters for CI, etc. Everyone can write something similar and publish as NPM package.

Moreover, we can ask @MoLow to add his own implementation (with package.json support or whatever) following the example of 'reporters' repo, which can be used as default solution for 80% of Node users. Sometimes this configuration tool may come bundled with Node, or deprecated in favour of 'full-featured configuration system for Node' in 2030.

Also run() currently doesn’t accept reporters.

It accepts - trough 'setup' callback: root.reporter.compose(reporter).pipe(your destination).

@MoLow
Copy link
Member

MoLow commented Jul 23, 2023

Moreover, we can ask @MoLow to add his own implementation (with package.json support or whatever) following the example of 'reporters' repo, which can be used as default solution for 80% of Node users. Sometimes this configuration tool may come bundled with Node, or deprecated in favour of 'full-featured configuration system for Node' in 2030.

I am pretty sure this is something that cannot happen in userland. flags are parsed before execution of any js code

@koshic
Copy link

koshic commented Jul 23, 2023

Moreover, we can ask @MoLow to add his own implementation (with package.json support or whatever) following the example of 'reporters' repo, which can be used as default solution for 80% of Node users. Sometimes this configuration tool may come bundled with Node, or deprecated in favour of 'full-featured configuration system for Node' in 2030.

I am pretty sure this is something that cannot happen in userland. flags are parsed before execution of any js code

You are definitely right... for regular Node usage. But 'run' is a bit different thing: each test file executed as separate process, so we can specify any options via process.env.NODE_OPTIONS = '...' then call 'run' - and it works.

Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Jan 20, 2024
Copy link
Contributor

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 20, 2024
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. stale
Projects
None yet
Development

No branches or pull requests

5 participants