Support defining test reporter using environment variable

[remcohaszing]

What is the problem this feature will solve?

#45712 added support for custom test reporters, which is great!

It’s still a but cumbersome having to specify the --test-reporter flag for every test run.

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

Use an environment variable NODE_TEST_REPORTER. If this is set, this will override the default tap runner. The command line argument will still take precedence. If this is specified, it will entirely ignore the NODE_TEST_REPORTER environment variable.

# Uses the tap formatter
node --test

# Uses the spec formatter
NODE_TEST_REPORTER=spec node --test

# Uses the tap formatter
NODE_TEST_REPORTER=spec node --test --test-reporter tap

This allows users to define their preferred formatter in their environment, e.g. in ~/.bashrc or ~/.zshrc

What alternatives have you considered?

It could also be supported in NODE_OPTIONS. However, people use this for different purposes too.

[cjihrig]

It's unlikely that we will add an environment variable for this. The general consensus is for things like that to be done via NODE_OPTIONS (otherwise we would end up supporting CLI flags and environment variables for every option supported by Node).

[MoLow]

@cjihrig --test-reporter is not allowed in NODE_OPTIONS and neither are any other test runner flags. perhaps we should change that

[cjihrig]

@MoLow sounds fine to me. The main reason for not doing so up to this point is that we'll need to parse the NODE_OPTIONS environment variable in the CLI runner when spawning child processes to make sure this logic is preserved.

[SRHerzog]

Re: the last couple of comments, I can dig into this if no one else is busy with it.

[remcohaszing]

The problem I see putting this in NODE_OPTIONS, is that a user may specify multiple --test-reporter and --test-reporter-destination flags, and order matters. Should these flags, when specified in NODE_OPTIONS, be discarded if the user passes these flags explicitly? Should they be appended?

Another solution could be to introduce a Node user configuration file ($XDG_CONFIG_HOME/node/config.json), but thats quite a big deviation from current approaches for options. This could contain other options, e.g. to configure the repl.

{
  "repl": {
    "ignoreUndefined": true
  },
  "test-reporter": [
    "spec",
    ["tap", "output.tap"]
  ]
}

[SRHerzog]

I think the intuitive behavior would be to ignore the values in NODE_OPTIONS when flags are passed via the CLI.

[cjihrig]

NODE_OPTIONS has documented behavior for such edge cases: https://nodejs.org/api/cli.html#node_optionsoptions

[SRHerzog]

This mostly works but will break on a --test-reporter or --test-reporter-destination value with a space in it, because I just split the NODE_OPTIONS string rather than properly parsing out quoted values. I can keep going to fully parse NODE_OPTIONS, assuming we want to make sure that values with spaces are supported properly.

[simoneb]

@SRHerzog why do we need to parse them again? Surely there must be a place in the codebase where the parsing is already done for us, and there are other options which support a space between the option name and the value, e.g. --require.

[SRHerzog]

NODE_OPTIONS is parsed in the C++ layer and merged with the CLI arguments, along with defaults where appropriate. It's read by the JavaScript layer as a Map after that merge is complete. There isn't a binding for the JavaScript runtime to retrieve NODE_OPTIONS in tokenized form or extract the NODE_OPTIONS values from the overall options map, so we would have to add that kind of functionality to node_options.cc if we want to avoid parsing the whole string again.

[simoneb]

Gotcha, I'm not familiar enough with the codebase to know when things happen exactly, I was just assuming that they must be happening somewhere. I assume that that when happens to late for us to make any use of it in this context. I wonder, aren't there other NODE_OPTIONS entries which the test runner is already making use of? How are they parsed?

[SRHerzog]

The only option specific to the test runner that can currently be passed through NODE_OPTIONS is --test-only. That one is passed through to child processes if it's set from the CLI and not filtered out like --test-reporter and --test-reporter-destination.

[simoneb]

Apologies Steve I'm just commenting without any good level of understanding of what's actually going on here. Why can't we do the same as with --test-only then?

[SRHerzog]

I think it's necessary for --test-reporter and --test-reporter-destination to not be passed through from the parent test process to the child test process, because the child must report its test output through stdout in the default TAP format in order to be parsed by the parent.

[simoneb]

Ok this makes more sense now, thanks for clarifying. I'm not particularly fond of the solution but I don't have any better ideas.

[SRHerzog]

I agree that parsing and filtering the command line args and NODE_OPTIONS seems hacky. A cleaner implementation would be to add another option to pass to the child process that causes it to override the normal logic for identifying test reporters and destinations.

async function setupTestReporters(testsStream) {
  const isChildTestProcess = getOptionValue('--test-child-process');
  const destinations = isChildTestProcess ? [kDefaultDestination] :
    getOptionValue('--test-reporter-destination');
  const reporters = isChildTestProcess ? [kDefaultReporter] :
    getOptionValue('--test-reporter');
  ...