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

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

Closed
rluvaton opened this issue May 10, 2023 · 19 comments
Closed

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

rluvaton opened this issue May 10, 2023 · 19 comments
Labels
feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.

Comments

@rluvaton
Copy link
Member

What is the problem this feature will solve?

part of the development and testing flow I add and remove .only.

The need to change the command each time makes testing and prototyping much slower and prevents you from using --watch command

What is the feature you are proposing to solve the problem?

Only have --test and in case there is only execute only those tests

What alternatives have you considered?

No response

@rluvaton rluvaton added the feature request Issues that request new features to be added to Node.js. label May 10, 2023
@MoLow MoLow added the test_runner Issues and PRs related to the test runner subsystem. label May 10, 2023
@MoLow
Copy link
Member

MoLow commented May 10, 2023

I do agree the DX is currently not great.

Implementing this feature would require a large change in how the test runner runs tests, and would have some performance penalty.
there are two big issues (that are basically the same - tests execute immediately):

  • when running multiple test files - each file is isolated, so if one file has a test with only - it does not affect the other test files (that are potentially already running)
  • even if we reduce the problem to a single file: the current design of the test runner is to run tests immediately when they are defined - so at the time of running a test we don't know yet if there are any tests with only.
    an exception for this behavior is direct children of describe - we first create all children of describe and only then do we run them (this is to avoid the need to await test/await it inside describe).

if we would want to detect only tests before any test runs - we would need to convert the entire test runner to work in such a two-phase manner: first, build the test tree. then execute it - depending on what we've learned from the tree structure.

I am not saying this isn't possible - but it probably a has significant performance cost, especially when running more than a single file.
I am currently -1 on implementing this unless there is a proposed solution that does not involve pre-building a test tree

@cjihrig
Copy link
Contributor

cjihrig commented May 10, 2023

I'm also strongly -1 to a solution that would come with a performance hit because "only" tests are not the common case. While needing the extra CLI argument is not the most ideal (but is also used by at least one other popular test runner), it makes it more difficult to accidentally bork your test suite by checking in only changes.

@rluvaton
Copy link
Member Author

what if --test-only will run everything if no it.only test found?

@cjihrig
Copy link
Contributor

cjihrig commented May 10, 2023

if no it.only test found?

You have to do a pass over all of the possible tests to know if it was found or not. That's what @MoLow was saying in #47945 (comment).

@rluvaton
Copy link
Member Author

yes I saw but we can pass twice instead of building the tree, and it will be a much easier solution if it's not common enough

@MoLow
Copy link
Member

MoLow commented May 10, 2023

so you suggest --run-only would mean "I am ok with the performance penalty"?

@rluvaton
Copy link
Member Author

rluvaton commented May 10, 2023

performance penalty can be solved later, but it makes it really hard to do some prototyping, first adoption is needed

@cjihrig
Copy link
Contributor

cjihrig commented May 10, 2023

what if --test-only will run everything

I don't think that this is something we can do. Not only would it be a breaking change, but it would be extremely confusing DX - far worse than asking to specify another CLI flag.

performance penalty can be solved later

I'm not so sure about this statement either. I don't think we can assume that this will just get figured out. If we had a plan, but not an implementation, that would be different. Yes, we could opt into a slower mode when --test-only is present, but you'd still be changing the CLI flags, and as I said, we can't run everything if there are no only tests. I don't think adding another CLI flag for "build the test tree, if there are only tests then run just those, otherwise run normally" is on the table either.

@MoLow
Copy link
Member

MoLow commented May 10, 2023

performance penalty can be solved later

the design restrictions I described are inherent. I think it can be mathematically proven that "performance penalty cannot be solved later"

@cjihrig
Copy link
Contributor

cjihrig commented May 14, 2023

I'm going to close this, but anyone is welcome to create a prototype with benchmark numbers.

@cjihrig cjihrig closed this as completed May 14, 2023
@rluvaton rluvaton changed the title having --test and --test-only flags reduce prototyping time having --test and --test-only flags increase prototyping time Jun 17, 2023
@machineghost
Copy link

machineghost commented Jul 23, 2023

What is the use case for the Node version of .only?

The use case in other major test frarmeworks is clear: I have 50 tests of one function, and I (temporarily) only want to see the output from one test, so I mark it as .only, and then I can see just that test's output.

If Node's version can't serve that use case, who does it serve?

@machineghost
Copy link

machineghost commented Jul 23, 2023

P.S. As far as performance, I'm curious how Mocha (see https://mochajs.org/#exclusive-tests), Jest (https://jestjs.io/docs/api#describeonlyname-fn), etc. all solved this same problem: surely Node's test runner that fundamentally different from all others?

@MoLow
Copy link
Member

MoLow commented Jul 26, 2023

reopening this since as time passes, I hear more people needing this and I do think it might be worth the penalty.
I have also started working on it

@MoLow MoLow reopened this Jul 26, 2023
@cjihrig
Copy link
Contributor

cjihrig commented Jul 26, 2023

@MoLow please provide benchmarks.

@rotty3000
Copy link

rotty3000 commented Jan 4, 2024

It is unfriendly to not have the convenience of executing single tests without having to change code. Every other test framework worth it's salt allows for this somehow. The performance penalty vs. the user pain of not being able to execute no more than what you want is not a good argument in my mind.

My purely non scientific experience tells me that node --test (even with the typescript loader) is already orders of magnitude faster then the other frameworks (notably; mocha and jest) even with a fairly small number of tests (we're talking like 20).

With that in mind adding a small pre-test-scan phase to collect all the test names (along with tracking a hierarchical naming convention) when a filtering argument is passed make sense to me (user opting into the cost of the pre-test-scan, I think users could stomach this).

Additionally, even today with --test-name-pattern there is a pretty nasty flaw which causes all before() to execute regardless of whether they are in any of the matching suites, which might be part of the issue you are trying to identify by running single tests btw... (i.e. test context poisoning for instance).

Finally, the reporters are noisy as heck when using --test-name-pattern because I still get all the skipped messages.

It should be obvious from the fact that the pattern argument was passed that we don't care to know that non-matching tests were skipped. Sure, if there are matching tests that were skipped because they were programmatically declared to be, then fine.

The point is that I can't adopt node --test if all these user experience pains don't get ironed out. It's too bad because node --test is really fast but still largely not usable in anger.

I urge the developers to take these things into consideration. Thanks.

@mohsen1
Copy link

mohsen1 commented Feb 28, 2024

I want --test to work with test.only just like Jest because that's what it makes sense to most developers. A few points from my experience with other frameworks

  • I only mark the test block with .only. the parents (describe blocks) and before/after hooks needed to run are automatically figured out by the testing framework
  • I don't want to see any output from the skipped test. The reason I mark a test with .only is to focus on that one test
  • Having to restart the test runner with a new CLI flag is really not ideal, I want to quickly focus on one test, fix it and unfocus to make sure now with the fix, all of the other tests are passing in the watch mode
  • A lot of folks have ESLint rule to disallow .only to sneak in the main branch via CI checks.

@monsanto
Copy link
Contributor

monsanto commented May 6, 2024

I've been using node's test runner for a bit now and this is a legitimate pain point. It would be more developer friendly if the default had the only/all tests behavior and there was a flag to reject only + run faster that can be used for CI or pre-push hooks. Whatever speed difference there would be for the dev loop is probably negated by the mental overhead of figuring out which command/flag to use. If there was a flag that one could put in a package.json script to give this behavior that would be fine too I guess but I think the current default is a mistake.

@cjihrig
Copy link
Contributor

cjihrig commented May 6, 2024

We are interested in having an option to run all tests in the same process. See #51579. Once that PR is merged, I think it will be possible to only require --test-only when using process level isolation.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 15, 2024

I'm going to close this issue. As of #54832 and #54881, --test-only is only required when using process level test isolation.

@cjihrig cjihrig closed this as completed Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants