Skip to content

test_runner: propagate only to test ancestors - #48932

Closed
MoLow wants to merge 5 commits into
nodejs:mainfrom
MoLow:test-runner-only
Closed

test_runner: propagate only to test ancestors#48932
MoLow wants to merge 5 commits into
nodejs:mainfrom
MoLow:test-runner-only

Conversation

@MoLow

@MoLowMoLow commented Jul 26, 2023

Copy link
Copy Markdown
Member

Fixes: #47945

this PR is broken into smaller commits that are intended to be logically independent and easier to review.

some limitations/todos:

  • compare benchmarks (benchmark: add benchmarks for the test_runner #48931)
  • This only works for a single file. meaning test.only will skip all tests in that file, but not in other files.
    solving this is much less trivial since it requires some kind of two-way communication between the test file and the parent process, in between the enqueue and the dequeue phases.
    we should consider if this solution is a big enough improvement compared to the current behavior for it to be released, or if we should solve multiple files as well
  • need to update documentation?
  • need to document deprecation of --run-only
  • add a test for --test-only with multiple files

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Jul 26, 2023
Comment threadlib/internal/test_runner/harness.js Outdated
@cjihrig

Copy link
Copy Markdown
Contributor

I haven't reviewed or tested this, but some initial thoughts:

This only works for a single file

This seems like it would be more confusing than what we currently have.

some kind of two-way communication between the test file and the parent process, in between the enqueue and the dequeue phases

I don't think this is going to end up in a very efficient design. Unless I'm mistaken, it also doesn't solve the problem of an only test that is not at the top level.

I think it would probably be better to keep the --test-only flag, if that flag is provided build the entire test tree, and then go back and run the only flags. I get that people don't want to specify --test-only, but it seems more powerful than the alternative here because you would not pay the extra cost in the common case and you could determine if there are nested only tests. Happy to be proven wrong on this though.

As a side note: I think solving #46728 would be more useful since it overlaps a lot with this feature and also makes IDE integration significantly better. I believe that issue can be solved in the current state of the test runner, but could also be solved by building the entire test tree.

@MoLow

Copy link
Copy Markdown
MemberAuthor

Unless I'm mistaken, it also doesn't solve the problem of an only test that is not at the top level.

you are mistaken (It might have a bug, but the intent is for a test at any level to cause all other tests to be skipped).
for example, this code results in this output:

describe('top level',()=>{describe('nested',()=>{describe('nested',()=>{it.only('only',(t)=>{t.diagnostic('only')});it('this is a test',()=>{});it('this is a test',()=>{});});it('this is a test',()=>{});});it('this is a test',()=>{});});it.only('this is a test',()=>{});it('this is a test',()=>{console.log('this is a test')});
▶ top level ▶ nested ▶ nested ✔ only (0.208458ms) ℹ only ﹣ this is a test (0.090208ms) # SKIP ﹣ this is a test (0.041084ms) # SKIP ▶ nested (1.021583ms) ﹣ this is a test (0.077125ms) # SKIP ▶ nested (1.2045ms) ﹣ this is a test (0.031ms) # SKIP▶ top level (1.404667ms)✔ this is a test (0.037875ms)﹣ this is a test (0.041542ms) # SKIPℹ tests 7ℹ suites 3ℹ pass 2ℹ fail 0ℹ cancelled 0ℹ skipped 5ℹ todo 0ℹ duration_ms 2.580958

I think it would probably be better to keep the --test-only flag, if that flag is provided build the entire test tree, and then go back and run the only flags. I get that people don't want to specify --test-only, but it seems more powerful than the alternative here because you would not pay the extra cost in the common case and you could determine if there are nested only tests. Happy to be proven wrong on this though.

doing this won't solve the multiple files issue.
the pain today is that you must specify {only:true} in each ancestor of a test that you really want to only run. once you eliminate that - even if you do that only behind the --test-only flag - you must handle tests in other files, otherwise all the tests in files that dont contain only tests will run

@MoLow
MoLowforce-pushed the test-runner-only branch from b03a866 to 6d4f641CompareJuly 28, 2023 05:37
@MoLowMoLow changed the title test_runner: propagate only to not need --test-onlytest_runner: propagate only to test ancestorsJul 28, 2023
@MoLow
MoLowforce-pushed the test-runner-only branch from 6d4f641 to 8e6435cCompareJuly 28, 2023 05:37
@MoLow

Copy link
Copy Markdown
MemberAuthor

@cjihrig Following our discussion, I have updated the pr

@MoLow
MoLowforce-pushed the test-runner-only branch 4 times, most recently from f71a3ed to e82455fCompareAugust 3, 2023 19:57
@MoLow

This comment was marked as outdated.

@MoLow

MoLow commented Aug 3, 2023

Copy link
Copy Markdown
MemberAuthor

seems like we might want to still require --test-only in single file execution, but bubble up the only tests when using --test-only

@MoLow
MoLowforce-pushed the test-runner-only branch 2 times, most recently from d3c2853 to c818aabCompareAugust 15, 2023 17:42
Comment threadbenchmark/test_runner/multi-files.js Outdated
@MoLow
MoLowforce-pushed the test-runner-only branch 2 times, most recently from 0e09ada to 0017181CompareAugust 16, 2023 11:38
@cjihrig

Copy link
Copy Markdown
Contributor

@MoLow are you still planning to work on this? I think it makes sense to implement this for suites now. If you don't want to, I'd be happy to take a shot at it.

@MoLow

Copy link
Copy Markdown
MemberAuthor

@MoLow are you still planning to work on this? I think it makes sense to implement this for suites now. If you don't want to, I'd be happy to take a shot at it.

Taking a look now

@MoLowMoLow closed this Mar 31, 2024
@MoLow
MoLow deleted the test-runner-only branch March 31, 2024 22:49
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.test_runnerIssues and PRs related to the test runner subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

having --test and --test-only flags increase prototyping time

4 participants

@MoLow@nodejs-github-bot@cjihrig@rluvaton