node icon indicating copy to clipboard operation
node copied to clipboard

test_runner: improve `--test-name-pattern` to allow matching single test

Open mdrobny opened this issue 4 months ago β€’ 6 comments

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 πŸ˜‰

mdrobny avatar Jan 27 '24 09:01 mdrobny

Review requested:

  • [ ] @nodejs/test_runner

nodejs-github-bot avatar Jan 27 '24 09:01 nodejs-github-bot

I fixed too long commit message, it should pass the lint now

mdrobny avatar Jan 27 '24 16:01 mdrobny

do we have a benchmark test for --test-name-pattern? the changes looks ok but I wonder what are the performance implications

MoLow avatar Jan 27 '24 20:01 MoLow

@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

mdrobny avatar Jan 28 '24 13:01 mdrobny

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)

image

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?

mdrobny avatar Jan 30 '24 22:01 mdrobny

@MoLow what do you think about the benchmark results?

mdrobny avatar Feb 15 '24 07:02 mdrobny

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

nodejs-github-bot avatar Feb 18 '24 11:02 nodejs-github-bot

@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?

mdrobny avatar Feb 26 '24 17:02 mdrobny

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

nodejs-github-bot avatar Feb 26 '24 19:02 nodejs-github-bot

@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 only some test in test 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

MoLow avatar Feb 26 '24 19:02 MoLow

@mdrobny please rebase this PR

MoLow avatar Feb 26 '24 20:02 MoLow

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

nodejs-github-bot avatar Feb 28 '24 16:02 nodejs-github-bot

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

nodejs-github-bot avatar Feb 29 '24 10:02 nodejs-github-bot

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Ε‚ Drobniak  (@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
https://github.com/nodejs/node/actions/runs/8096266840

nodejs-github-bot avatar Feb 29 '24 12:02 nodejs-github-bot

Landed in 00dc6d9d97dcba61aa52ae2eb315ec8e2a81e1f8

nodejs-github-bot avatar Mar 01 '24 12:03 nodejs-github-bot