node
node copied to clipboard
test_runner: improve `--test-name-pattern` to allow matching single test
Try to match a test by name prefixed with all its ancestors to ensure uniqueness of the name
Fixes: https://github.com/nodejs/node/issues/46728
It adds a feature that is present in all major test runners. It's well described in this comment
It allows developers to quickly run/debug a single test e.g. from their editor
I am a first time contributor π
Review requested:
- [ ] @nodejs/test_runner
I fixed too long commit message, it should pass the lint now
do we have a benchmark test for --test-name-pattern
? the changes looks ok but I wonder what are the performance implications
@MoLow you are right, there can be negative performance implications when using --test-name-pattern
flag since we must do additional regexp matching for all not matching tests.
I can try to write some naive benchmark test. So far, for 1000 test suits, I didn't observe any meaningful difference
I experimented with benchmarking test runner with mitata package. It repeats many times the operation and does the measurements
Here is the test suite repeated 1000 times
const {describe, it} = require('node:test');
for (let i = 0; i < 1000; i++) {
describe('describeTest', () => {
it('itTest1', () => {});
it('itTest2', () => {});
describe('describeTest', () => {
it('itTest1', () => {});
it('itTest2', () => {});
});
})
}
here is the benchmark code using my locally compiled version of Node with changes from this branch
import { run, bench, group, baseline } from 'mitata';
import { spawnSync } from 'node:child_process'
const pathToLocalNode = '../../node/node';
group('test runner', () => {
baseline('no pattern', () => {
spawnSync(pathToLocalNode, ['--test'], { stdio: 'ignore' })
});
bench('with name pattern not matching any test', () => {
spawnSync(pathToLocalNode, ['--test-name-pattern', 'nothing', '--test'], {stdio: 'ignore'})
})
bench('with name pattern matching all tests', () => {
spawnSync(pathToLocalNode, ['--test-name-pattern', 'describeTest', '--test'], {stdio: 'ignore'})
})
bench('with name pattern matching single test', () => {
spawnSync(pathToLocalNode, ['--test-name-pattern', 'describeTest describeTest itTest2', '--test'], { stdio: 'ignore' })
})
});
await run();
those are the results from running benchmark on my computer (Macbook with M1 Pro)
Looks like there is almost no difference when using --test-name-pattern
flag which is.. surprising π€ I expected a bit more slowness. Am I missing something in this benchmark?
@MoLow what do you think about the benchmark results?
CI: https://ci.nodejs.org/job/node-test-pull-request/57177/
@MoLow why do you consider it a major (breaking) change? π€
it rather won't affect current usages of --test-name-pattern
but adds support for matching single test by prefixing with all parent names.
Do you have some example in mind where this will break some current usage?
CI: https://ci.nodejs.org/job/node-test-pull-request/57440/
@MoLow why do you consider it a major (breaking) change? π€ it rather won't affect current usages of
--test-name-pattern
but adds support for matching single test by prefixing with all parent names. Do you have some example in mind where this will break some current usage?
using the example from the documentation, added in this PR:
describe('test 1', (t) => {
it('some test');
});
describe('test 2', (t) => {
it('some test');
});
Starting Node.js with
--test-name-pattern="test 1 some test"
would match onlysome test
intest 1
.
prior to this change, using --test-name-pattern="test 1 some test"
would run no tests, and now it will run 1 test - I consider that a breaking change
@mdrobny please rebase this PR
CI: https://ci.nodejs.org/job/node-test-pull-request/57490/
CI: https://ci.nodejs.org/job/node-test-pull-request/57498/
Commit Queue failed
- Loading data for nodejs/node/pull/51577 β Done loading data for nodejs/node/pull/51577 ----------------------------------- PR info ------------------------------------ Title test_runner: improve `--test-name-pattern` to allow matching single test (#51577) Author MichaΕ Drobniakhttps://github.com/nodejs/node/actions/runs/8096266840(@mdrobny, first-time contributor) Branch mdrobny:feat/test-runner-match-unique-name -> nodejs:main Labels semver-major, author ready, needs-ci, test_runner Commits 1 - test_runner: improve `--test-name-pattern` to allow matching single test Committers 1 - mdrobny PR-URL: https://github.com/nodejs/node/pull/51577 Fixes: https://github.com/nodejs/node/issues/46728 Reviewed-By: Moshe Atlow Reviewed-By: Chemi Atlow ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/51577 Fixes: https://github.com/nodejs/node/issues/46728 Reviewed-By: Moshe Atlow Reviewed-By: Chemi Atlow -------------------------------------------------------------------------------- βΉ This PR was created on Sat, 27 Jan 2024 09:22:46 GMT β Approvals: 2 β - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/51577#pullrequestreview-1908557778 β - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/51577#pullrequestreview-1887168630 β semver-major requires at least 2 TSC approvals β Last GitHub CI successful βΉ Last Full PR CI on 2024-02-29T10:49:29Z: https://ci.nodejs.org/job/node-test-pull-request/57498/ - Querying data for job/node-test-pull-request/57498/ β Last Jenkins CI successful -------------------------------------------------------------------------------- β Aborted `git node land` session in /home/runner/work/node/node/.ncu
Landed in 00dc6d9d97dcba61aa52ae2eb315ec8e2a81e1f8