Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test runner should allow specific subtest filtering, via --test-name-pattern or otherwise #46728

Closed
connor4312 opened this issue Feb 19, 2023 · 18 comments · Fixed by #51577
Closed
Labels
test_runner Issues and PRs related to the test runner subsystem.

Comments

@connor4312
Copy link
Contributor

connor4312 commented Feb 19, 2023

Version

19.3.0

Platform

Darwin MacBook-Pro-2.local 22.3.0 Darwin Kernel Version 22.3.0: Thu Jan 5 20:53:49 PST 2023; root:xnu-8792.81.2~2/RELEASE_X86_64 x86_64

Subsystem

test_runner

What steps will reproduce the bug?

Given a set of tests like this

describe("my suite 1", () => {
  it("works", async () => {
    // don't run me
  });

  describe("my suite 2", () => {
    it("works", async () => {
      // run me!
    });
  })
});

... it is impossible to command line filter (--test-name-filter) to run only the "works" test in my suite 2.

Being able to be precise about tests to run is important when working on an editor integration. If a user asks to "run this test", then only that test should run. This is especially relevant in the use case of integration tests, where running tests may have side effects.

How often does it reproduce? Is there a required condition?

100%

What is the expected behavior?

I should be able to run my suite 2 > works by itself. In every test runner I've worked with (jest, mocha, or vitest for example) names are generated for glob purposes by space-delimiting nested tests. So I expected to be able to pass --test-name-pattern="^my suite 1 my suite 2 works$" to run only that test.

(This does mean that identically named tests in the same context cannot be differentiated. But this is a common problem across test runners that also affects result output, and I already have code that emits a diagnostic warning ('yellow squiggle') if the user does this.)

What do you see instead?

I must pass --test-name-pattern="^my suite 1$" --test-name-pattern="^my suite 2$" --test-name-pattern="^works$", which runs the undesired test.

Additional information

Apologies for the late followup on #42984. I didn't get a chance to work on the vscode integration for this for a few months, and only noticed this issue later.

@connor4312 connor4312 changed the title Test runner should allow specific subtest filtering Test runner should allow specific subtest filtering, via --test-name-pattern or otherwise Feb 19, 2023
@cjihrig
Copy link
Contributor

cjihrig commented Feb 19, 2023

I think we should be able to support this without any additional overhead other than the case where --test-name-pattern is used. In this block we already compare the test name against all of the user provided patterns. We could also split the user provided patterns on some separator (> in the example above) and using the parent pointer, loop until we know if we have a fully qualified match or not.

@MoLow MoLow added the test_runner Issues and PRs related to the test runner subsystem. label Feb 19, 2023
@richiemccoll
Copy link
Contributor

I'll take a look at this if nobody else has started on it.

@loynoir
Copy link

loynoir commented Mar 28, 2023

Hi guys, how about test on concated names?

var describeNames = ['level1', 'level2'];
var itName = 'should pass'
var testNamePattern = /level1>level2>should pass/

testNamePattern.test([...describeNames,itName].join('>'))
// true

I feel simpler and more reliable than inventing pattern syntax.

@MoLow
Copy link
Member

MoLow commented Jun 5, 2023

I think describe should not be filtered, and we should only filter tests, @cjihrig WDYT?

@connor4312
Copy link
Contributor Author

All other runners that come to mind do filter both on describe and test as part of the same sequence, as in the example in the issue report.

@cjihrig
Copy link
Contributor

cjihrig commented Jun 6, 2023

I think describe should not be filtered, and we should only filter tests, @cjihrig WDYT?

I would expect describes to be included.

@MoLow
Copy link
Member

MoLow commented Jun 7, 2023

I have checked, and these are the behaviors of other test runners:

  • jest --testNamePattern always runs describe regardless of the pattern.
  • mocha --grep - runs all describe regardless of the pattern.
  • playwright --grep - runs all describe regardless of the pattern.
  • ava, tape, node-tap - no describe

All other runners that come to mind do filter both on describe and test as part of the same sequence, as in the example in the issue report.

this seems to be incorrect @connor4312

I would expect describes to be included.

@cjihrig do you still expect that, given this information?

@cjihrig
Copy link
Contributor

cjihrig commented Jun 7, 2023

That would still be my expectation, but I guess I would be wrong 😄. I'm fine with following the behavior of everyone else here.

@targos
Copy link
Member

targos commented Jun 7, 2023

Jest certainly supports filtering by describe. It's the main way I work with it.

@connor4312
Copy link
Contributor Author

connor4312 commented Jun 7, 2023

Ah, sorry, I misunderstood. describes are run since the grep pattern is a pattern, not a prefix, so all tests have to get enumerated by running all describe blocks. So yes I think your intention is correct in saying "describe should not be filtered"

@MoLow
Copy link
Member

MoLow commented Jun 7, 2023

Ok, so In regards to the comment made by @targos, I have tested this:

describe("yes", function() {
  it("no", () => {});
  it("yes", () => {});

  describe("maybe", function() {
    it("no", () => {});
    it("yes", () => {});
  });
});

describe("no", function() {
  it("no", () => {});
  it("yes", () => {});

  describe("maybe", function() {
    it("no", () => {});
    it("yes", () => {});
  });
});
  • playwright test --grep "yes": all ran besides no -> no, no -> maybe -> no
  • mocha --grep "yes": all ran besides no -> no, no -> maybe -> no
  • jest --testNamePattern=yes: all ran besides no -> no, no -> maybe -> no

meaning describe can be used as a filter, but only to rule in its descendants - not to rule them out

@DmitryMakhnev
Copy link

DmitryMakhnev commented Oct 16, 2023

Hello,

I'm implementing a Node.js test runner integration in WebStorm.
I would like to save common developers workflows for the test runner integration.

One of these common workflows involves running a special test from a file that contains multiple tests.
This can be particularly useful when debugging a specific use case:

debug-selected-test.mov

It's an example for jest

To implement this workflow, it's essential to have the ability to configure a test runner to execute a specific test.
Unfortunately, I couldn't find a way to do this with the current --test-name-pattern implementation.

Code Example

Let's imagine that we have tests for callback and promise-based functionality (inspired by test-fs-write-file-flush.js from the Node.js repository):

describe(`callback version`, () => {
  test(`valid`, () => new Promise((resolve, reject) => {
    validatePositiveNumber(1, (error, isValid) => {
      try {
        assert.ok(isValid);
        resolve();
      } catch (e) {
        reject(e);
      }
    });
  }));

  // invalid, etc
});

describe(`promise based version`, () => {
  test(`valid`, () =>
    validatePositiveNumber(1)
      .then(isValid => {
        assert.ok(isValid);
      })
  );

  // invalid, etc
});

Let's attempt to filter the valid test case from the callback version suite,
using Node.js 20.4+ with the merged 'test_runner: make --test-name-pattern recursive #48382' PR.

  • Filtering with --test-name-pattern="callback version" will run all tests in callback version suit (example).
  • Filtering with --test-name-pattern="valid" will run all tests in the callback version and the promise based version suites because they contain valid test cases (example).
  • Filtering with 2 --test-name-pattern parameters --test-name-pattern="callback version" and --test-name-pattern="valid" also runs all tests in this case. (example).
    However, for Node.js versions earlier than 20.4, this approach works with the code example (example).
  • Filtering with --test-name-pattern and combined names as ^callback version valid$ doesn't run any test. (example).

Can you please suggest a way to filter the valid test cases from the callback version suite in this code example?

The example in other tests runners

I checked the example in other test runners, and here are the results:

Suggestion to extend --test-name-pattern

So it appears that in general if some test runner supports grouping by describe(), the runner gives a way to filter a specific test through all levels of the groups. Perhaps we should consider extending the --test-name-pattern parameter to support this type of filtration more commonly.

After reviewing other test runners, it appears that we can simply extend the --test-name-pattern to support the full path to the test. For the code example provided, it would look like ^callback version valid$. This solution allows us to address the issue without requiring any breaking changes and offers a straightforward and widely-adopted approach.

Could please say what you think about it?

mdrobny added a commit to mdrobny/node that referenced this issue Jan 27, 2024
Try to match a test by name prefixed with all its ancestors to ensure uniqueness of the name

Fixes: nodejs#46728
mdrobny added a commit to mdrobny/node that referenced this issue Jan 27, 2024
Try to match a test by name prefixed with all its ancestors to ensure uniqueness of the name

Fixes: nodejs#46728
@mdrobny
Copy link
Contributor

mdrobny commented Jan 27, 2024

@connor4312 @DmitryMakhnev I made a PR that I think directly addresses this issue 🤞🏼

mdrobny added a commit to mdrobny/node that referenced this issue Jan 27, 2024
Try to match a test by name prefixed with all its ancestors to ensure uniqueness of the name

Fixes: nodejs#46728
mdrobny added a commit to mdrobny/node that referenced this issue Jan 27, 2024
Try to match a test by name prefixed with all its ancestors
to ensure uniqueness of the name

Fixes: nodejs#46728
@DmitryMakhnev
Copy link

Hello @mdrobny,

Thank you so much for your contribution! I'm looking forward these fixes in next version of Node.js.

Have a lovely time ✨

mdrobny added a commit to mdrobny/node that referenced this issue Feb 28, 2024
Try to match a test by name prefixed with all its ancestors
to ensure uniqueness of the name

Fixes: nodejs#46728
mdrobny added a commit to mdrobny/node that referenced this issue Feb 29, 2024
Try to match a test by name prefixed with all its ancestors
to ensure uniqueness of the name

Fixes: nodejs#46728
mdrobny added a commit to mdrobny/node that referenced this issue Feb 29, 2024
Try to match a test by name prefixed with all its ancestors
to ensure uniqueness of the name

Fixes: nodejs#46728
@mdrobny
Copy link
Contributor

mdrobny commented Feb 29, 2024

Update

So because this change is considered a breaking change, it will be released in Node.js 22 in April (if PR gets merged before that 😄 🤞🏼 )

mdrobny added a commit to mdrobny/node that referenced this issue Feb 29, 2024
Try to match a test by name prefixed with all its ancestors
to ensure uniqueness of the name

Fixes: nodejs#46728
nodejs-github-bot pushed a commit that referenced this issue Mar 1, 2024
Try to match a test by name prefixed with all its ancestors
to ensure uniqueness of the name

Fixes: #46728
PR-URL: #51577
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@connor4312
Copy link
Contributor Author

Thank you! I will eagerly adopt it as soon as possible 🙂

rdw-msft pushed a commit to rdw-msft/node that referenced this issue Mar 26, 2024
Try to match a test by name prefixed with all its ancestors
to ensure uniqueness of the name

Fixes: nodejs#46728
PR-URL: nodejs#51577
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@DmitryMakhnev
Copy link

Hello @mdrobny,
I Implemented support for your fixes for WebStorm and all JetBrains IDE's. With your fixes all works perfect!
Thank you so much.
Have a lovely time.

@connor4312
Copy link
Contributor Author

Adopted for VS Code in connor4312/nodejs-testing@a490ae2, works great 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
8 participants