Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
test_runner: propagate only to test ancestors#48932
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
MoLow commented Jul 26, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
nodejs-github-bot commented Jul 26, 2023
Review requested:
|
Uh oh!
There was an error while loading. Please reload this page.
cjihrig commented Jul 26, 2023
I haven't reviewed or tested this, but some initial thoughts:
This seems like it would be more confusing than what we currently have.
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 I think it would probably be better to keep the 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 commented Jul 27, 2023
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). 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
doing this won't solve the multiple files issue. |
only to not need --test-onlyMoLow commented Jul 28, 2023
@cjihrig Following our discussion, I have updated the pr |
f71a3ed to e82455fCompare This comment was marked as outdated.
This comment was marked as outdated.
MoLow commented Aug 3, 2023
seems like we might want to still require |
d3c2853 to c818aabCompareUh oh!
There was an error while loading. Please reload this page.
0e09ada to 0017181Comparecjihrig commented Mar 29, 2024
@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 commented Mar 31, 2024
Taking a look now |
Fixes: #47945
this PR is broken into smaller commits that are intended to be logically independent and easier to review.
some limitations/todos:
This only works for a single file. meaningtest.onlywill 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 document deprecation of--run-only--test-onlywith multiple files