Repository navigation
Support defining test reporter using environment variable #46484
Description
Activity
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).Reacted by Ben Noordhuis and Juan José@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_OPTIONSenvironment variable in the CLI runner when spawning child processes to make sure this logic is preserved.Reacted by Moshe AtlowRe: the last couple of comments, I can dig into this if no one else is busy with it.
Reacted by Moshe AtlowThe problem I see putting this in
NODE_OPTIONS, is that a user may specify multiple--test-reporterand--test-reporter-destinationflags, and order matters. Should these flags, when specified inNODE_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"] ] }I think the intuitive behavior would be to ignore the values in NODE_OPTIONS when flags are passed via the CLI.
NODE_OPTIONShas documented behavior for such edge cases: https://nodejs.org/api/cli.html#node_optionsoptionsReacted by Remco HaszingThis mostly works but will break on a
--test-reporteror--test-reporter-destination valuewith 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.@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.
Reacted by Moshe AtlowNODE_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.ccif we want to avoid parsing the whole string again.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?
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-reporterand--test-reporter-destination.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?
I think it's necessary for
--test-reporterand--test-reporter-destinationto 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.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.
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'); ...- added a commit that references this issue
on Mar 14, 2023 - added a commit that references this issue
on Mar 18, 2023 - added a commit that references this issue
on Jul 6, 2023
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-reporterflag 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 defaulttaprunner. The command line argument will still take precedence. If this is specified, it will entirely ignore theNODE_TEST_REPORTERenvironment variable.This allows users to define their preferred formatter in their environment, e.g. in
~/.bashrcor~/.zshrcWhat alternatives have you considered?
It could also be supported in
NODE_OPTIONS. However, people use this for different purposes too.