test_runner: remove problematic uses of parseCommandLine() #54353

cjihrig · 2024-08-13T13:44:44Z

This PR removes uses of parseCommandLine() throughout the test runner. This work is related to #53867, as it reduces the use of process global state in the test runner where it should not be used.

test_runner: use `run()` argument names in `parseCommandLine()`

This commit updates parseCommandLine() to use the names supported
by run(). This removes some unnecessary renaming code, and allows
node:test and run() to more easily share code.

test_runner: pass harness object as option to root test

This commit initializes the root harness object before the root
test and passes the harness as an option to the root test
constructor. This commit also attaches the global configuration
to the harness. This will allow the parseCommandLine() call in
test.js to be removed, as those values are now available via
the root test.

test_runner: pass global options to `createTestTree()`

The global configuration should already be known when
createTestTree() is called. This commit updates that function
to take the global configuration as an input.

test_runner: return `setup()` from `parseCommandLine()`

Now that parseCommandLine() returns run() compatible arguments,
it makes sense to return setupTestReporters() as the setup()
argument to run(). This also removes another problematic use of
parseCommandLine() in setupTestReporters().

test_runner: refactor hook creation

This commit makes hook creation more consistent by always
passing in a reference to the test that owns the hook. It also
removes some unnecessary validation on internal API.

test_runner: remove `parseCommandLine()` from `test.js`

The lib/internal/test_runner/test.js file should not use the
parseCommandLine() function. This commit refactors the code to
avoid doing so.

This commit updates parseCommandLine() to use the names supported by run(). This removes some unnecessary renaming code, and allows node:test and run() to more easily share code.

This commit initializes the root harness object before the root test and passes the harness as an option to the root test constructor. This commit also attaches the global configuration to the harness. This will allow the parseCommandLine() call in test.js to be removed, as those values are now available via the root test.

The global configuration should already be known when createTestTree() is called. This commit updates that function to take the global configuration as an input.

Now that parseCommandLine() returns run() compatible arguments, it makes sense to return setupTestReporters() as the setup() argument to run(). This also removes another problematic use of parseCommandLine() in setupTestReporters().

This commit makes hook creation more consistent by always passing in a reference to the test that owns the hook. It also removes some unnecessary validation on internal API.

The lib/internal/test_runner/test.js should not use the parseCommandLine() function. This commit refactors the code to avoid doing so.

nodejs-github-bot · 2024-08-13T13:44:49Z

Review requested:

@nodejs/test_runner

codecov · 2024-08-13T14:58:23Z

Codecov Report

Attention: Patch coverage is 96.55172% with 5 lines in your changes missing coverage. Please review.

Project coverage is 87.10%. Comparing base (d0f5943) to head (44bfa3b).
Report is 409 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/test_runner/utils.js	88.88%	3 Missing ⚠️
lib/internal/test_runner/test.js	96.77%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54353      +/-   ##
==========================================
+ Coverage   87.09%   87.10%   +0.01%     
==========================================
  Files         648      648              
  Lines      182213   182244      +31     
  Branches    34961    34958       -3     
==========================================
+ Hits       158703   158751      +48     
+ Misses      16794    16784      -10     
+ Partials     6716     6709       -7

Files with missing lines	Coverage Δ
lib/internal/test_runner/harness.js	`89.13% <100.00%> (+1.07%)`	⬆️
lib/internal/test_runner/runner.js	`85.76% <100.00%> (+0.20%)`	⬆️
lib/internal/test_runner/test.js	`98.35% <96.77%> (+0.18%)`	⬆️
lib/internal/test_runner/utils.js	`61.11% <88.88%> (ø)`

... and 23 files with indirect coverage changes

nodejs-github-bot · 2024-08-13T19:33:24Z

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

mcollina

lgtm

nodejs-github-bot · 2024-08-15T13:52:58Z

Landed in e020dd8...48d63c4

This commit updates parseCommandLine() to use the names supported by run(). This removes some unnecessary renaming code, and allows node:test and run() to more easily share code. PR-URL: #54353 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

This commit initializes the root harness object before the root test and passes the harness as an option to the root test constructor. This commit also attaches the global configuration to the harness. This will allow the parseCommandLine() call in test.js to be removed, as those values are now available via the root test. PR-URL: #54353 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

The global configuration should already be known when createTestTree() is called. This commit updates that function to take the global configuration as an input. PR-URL: #54353 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Now that parseCommandLine() returns run() compatible arguments, it makes sense to return setupTestReporters() as the setup() argument to run(). This also removes another problematic use of parseCommandLine() in setupTestReporters(). PR-URL: #54353 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

This commit makes hook creation more consistent by always passing in a reference to the test that owns the hook. It also removes some unnecessary validation on internal API. PR-URL: #54353 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

The lib/internal/test_runner/test.js should not use the parseCommandLine() function. This commit refactors the code to avoid doing so. PR-URL: #54353 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

MoLow · 2024-08-16T00:44:26Z

Nice 👍🏻 thanks for the effort