junit-team / junit5

✅ The 5th major version of the programmer-friendly testing framework for Java and the JVM
https://junit.org
Eclipse Public License 2.0
6.39k stars 1.48k forks source link

Discover dynamic tests early during discovery phase #1338

Closed mibutec closed 6 years ago

mibutec commented 6 years ago

As discussed in https://stackoverflow.com/questions/49401619/test-discovery-of-dynamic-tests-in-eclipse-slow it would be cool, if dynamic tests could be discovered at start time of a test. Advantages are:

Since a TestFactory returns a tupel of display name and executable code, I think it should be possible to iterate throu the returned Stream / Iterable to get all display names and create complete view on test start of a test.

sormuras commented 6 years ago

@mibutec Please read and understand the initial paragraph of https://junit.org/junit5/docs/current/user-guide/#writing-tests-dynamic-tests

Dynamic tests are not static. The framework will never be able to know what happens to the stream of DynamicNodes. A stream is not a collection, i.e. it does not have a pre-defined size, it does not even store data/items/tuples...

Closing this request as invalid.

sormuras commented 6 years ago

Maybe you're looking for Parameterized Tests?

mibutec commented 6 years ago

I'm sorry if I hurt you in any kind. I didn't mean to be rough.

I understand the differnce if a test needs to be known at compile time or if it needs to be known on runtime. But I think even tests defined at runtime should be known at some point in test execution, so I see there is no problem to go throu the stream as far as it's known to provide test descriptions for IDE. Maybe I'm just lacking of fantasy what dynamic tests can be used for, but I think 99% of all streams are fully known after @TestFactory annotated method finished execution.

Maybe I'm wrong, but I would like to discuss that idea. I don't see it's that aside, that it couldn't be even discussed and my issue is immediatly closed.

sormuras commented 6 years ago

I'm sorry if I hurt you in any kind. I didn't mean to be rough.

No offense taken, no offense meant from my side, too.

Let's discuss the idea here. Maybe the documentation and user-guide lacks detailed information regarding the purpose of the dynamic test feature. If this is the case, we need to improve it.

mibutec commented 6 years ago

My use case I is:

We use a lot of JBehave in my company. JBehave is a way to describe Tests in a Behavior Driven Design way. Tests are described as Scenarios in story files. In our environment several story files are executed in one JUnit test class.

Marriage between JBehave and JUnit was hard until now. In JUnit 5 I want to say a test factory: "Take a look at those story files, create tests out of them". Then several tests are created out of that story files each named as the scenario it executes. Inside the Executable there is some code utilizing JBehave core classes to run one scenario each test. For me that's one case to use Dynamic Tests, isn't it?

That all works fine so far. But one common use case is: I need to execute just one scenario out of a big story. This tests may be quite slow, so I really just want to execute that one test. In my JUnit 5 dream world I initiate test execution for the whole test, get a list of scenarios in my IDE's view, cancel test execution for the whole test class and run just the test I need. This won't work, this IDE's view doesn't know all the scenarios on test start, but after execution.

sormuras commented 6 years ago

Perhaps this "picture" will help to clarify the difference between static and dynamic tests.

Static / Discovery

Dynamic / Execution

sormuras commented 6 years ago

[...] so I really just want to execute that one test.

Create a dedicated @Test method to pin-point the single configuration?

sormuras commented 6 years ago

[...] For me that's one case to use Dynamic Tests, isn't it?

True. But when you chose a dynamic tool, you'll lose static feature.

When you chose reflection, you'll lose strong static features.

sormuras commented 6 years ago

Marriage between JBehave and JUnit was hard until now. In JUnit 5 I want to say a test factory: "Take a look at those story files, create tests out of them". Then several tests are created out of that story files each named as the scenario it executes. Inside the Executable there is some code utilizing JBehave core classes to run one scenario each test.

Hm, implement your own TestEngine? Here you can control what is a test and how they are executed.

Here are some starting points:

And when you're done, contribute it to https://github.com/jbehave/jbehave-core/jbehave-junit-engine 🚀

mibutec commented 6 years ago

TestEngine sounds like a good heading. I'll take a look at this. Thanks

mibutec commented 6 years ago

True. But when you chose a dynamic tool, you'll lose static feature.

I don't think this needs to be true in context of our discussion.

You get a Stream of names and executables out of a test factory. After the test factory is called you may go throu the stream and find all the display names to create a test description. The executables are used later.

This will work for all of your examples in https://junit.org/junit5/docs/current/user-guide/#writing-tests-dynamic-tests even generateRandomNumberOfTests(). The only case this wouldn't work is a test factory providing some tests, then taking some time to recap the tests created so far and than while the first tests already were executed, to notice: "Ahh, I forgot one, thanks god I have a stream, so I can hand in it later".

Sorry for sarcasm. I didn't find a use case for a test factory not knowning all tests when it finishes work. Can you give an example?

For me the big difference between static and dynamic tests is: Static tests need to be defined at compile time, dynamic ones not. That's already a huge, huge plus in comparsion with older JUnit versions.

sbrannen commented 6 years ago

Sorry for sarcasm. I didn't find a use case for a test factory not knowning all tests when it finishes work. Can you give an example?

Data being streamed in from an external resource: database, REST server, message queue, etc.

Although it's true that a test factory should not stream dynamic nodes indefinitely, the point is that such tests are dynamic in nature.

And "dynamic" tests are not discovered until the execution phase in the JUnit Platform.

For dynamic tests, only the test factory method is discovered during the discovery phase.

Note that the discovery phase precedes the execution phase, and it is during the discovery phase that the GUI in your IDE is populated with statically discovered tests.

Dynamic tests are not known until the execution phase and therefore cannot be displayed in the IDE prior to that.

mibutec commented 6 years ago

Thanks for clarification. By introducing that two phases, I got the problem.

sbrannen commented 6 years ago

Thanks for clarification. By introducing that two phases, I got the problem.

You're welcome!

Glad to be of service. 😃

mibutec commented 6 years ago

The more I think about it the less I get the point with the actual definition what a Dynamic Test is.

Data being streamed in from an external resource: database, REST server, message queue, etc.

For me that's an example of a datasources for parametrized tests.

For me the difference between a parametrized test and a dynamic test is:

As examples of Dynamic Tests I think of something like:

After working a bit with TestEngines, I get deeper into the idea of discovery phase and execution phase and I think one must destinguish what can be discovered early and what needs to wait for discovery.

Parameterized and Dynamic Tests have runtime parts necessary to do the whole discovery. But Runtime doesn't necessaryly mean slow and far, far away:

So I think you cannot say: "Dynamic Tests are always discovered in execution phase" or "Parameterized tests are always discovered in discovery phase". But it depends on the Runtime nature of your "runtime data provider".

The Framework cannot know about the nature of your "runtime data provider", it must be told by test developer. So maybe you can think of some meta information telling the Framework when to discover tests (@TestFactory(earlyDiscovery=true)) or you might make it dependent on the used return type (Stream<DynamicTest>) vs Collection<DynamicTest>)

mibutec commented 6 years ago

Why do I tell all this to you?

I had the last years a hard time getting some things done in JUnit 4 because the static nature of tests there. Now you come to my rescue, but what do I see: You are too dynamic.

I'd really love to have the ability to define a test without the need doing it at compile time with no other drawbacks, just behaving like any other static test.

sbrannen commented 6 years ago

The Framework cannot know about the nature of your "runtime data provider", it must be told by test developer. So maybe you can think of some meta information telling the Framework when to discover tests (@TestFactory(earlyDiscovery=true)) or you might make it dependent on the used return type (Stream<DynamicTest>) vs Collection<DynamicTest>)

That unfortunately will not help.

It is literally impossible to discover dynamic tests without executing the test factory method.

The execution of anything testable (e.g., @Test, @TestFactory, @ParameterizedTest methods) requires that the entire execution phase infrastructure be up and running.

Specifically, this means that all extensions must be discovered, instantiated, registered, and executed for anything testable. JUnit also must invoke all user-defined lifecycle callback methods (e.g., @BeforeAll, @BeforeEach, etc.).

So, in summary, although I understand your desire for the framework to invoke your test factory method during the discovery phase, that isn't possible since it physically requires execution of the test factory method, and "execution" is not as simple as just "invoking" the method.

sormuras commented 6 years ago

So, a custom test engine might be a solution here.

sbrannen commented 6 years ago

To make my above comments clear, I have renamed this issue to "Discover dynamic tests early during discovery phase" in order to reflect what is actually being requested.

mibutec commented 6 years ago

So, a custom test engine might be a solution here.

It is. So I'm on it. For now I'm trying to find out if a TestEngine is too much for what I want to achieve, or if it's making everything much better.

In the last years we arranged doing JBehave in a JUnit way, so with this in mind I don't need anything more than some Dynamic Tests.

Having now the possiblilty creating an API from scratch to execute those test, things are getting interesting.

sbrannen commented 6 years ago

Having now the possiblilty creating an API from scratch to execute those test, things are getting interesting.

Cool! 🤓

Let us know how it goes...

mibutec commented 6 years ago

It is literally impossible to discover dynamic tests without executing the test factory method.

That's clear, and now I get the problem: @TestFactory method might depend on being initialized by some Extension.

In case of @MethodSource you solved the problem by making that method static. Could that be a solution? Instead @TestFactory(earlyDiscovery=true) the keyword static in method declaration might be a signal word to allow early discovery.

I know I suck, but Dynamic Tests for me is the killer features of JUnit5, and I really was disapointed if them failed on such trivia like late feedback for developers what tests are going to be executed.

bechte commented 6 years ago

@mibutec To me it sounds like you are missing an Pointcut that allows you to take a discovered TestDescriptor and do some additional (even though limited) work during the discovery phase, e.g. like adding additional TestDescriptors that are resolved outside of the test Class. Currently, we are not supporting this, unfortunately.

As I was investigating quite some time in this direction, I do have a question: Would it be sufficient for your use-case, if you could implement some kind of TestProvider that is notified about every TestDescriptor found and to be able to attach children, i.e. instances of TestDescriptor to that node?

For sure, the children would be instances of TestDescriptor themselves and they would have to perform the test execution, and the test discovery by ID themselves in order to contribute.

And here comes the crucial part: We could run into conflicts if multiple TestProviders want to operate and modify the same node. This is something we could solve by limiting the richness of this feature, but there is something else in the air. Nowadays, the Jupiter engine knows in detail which constellations of TestDescriptors are valid, i.e. which follows which and who is a parent of who. In a flexible mechanism like proposed above, this kind of guarantee is missing. This also means that the whole part of Jupiters Extensions and how they are guaranteed to be executed, will be very hard to achieve. The TestDescriptors would have much more responsibility and dependencies.

If we find a way to solve the issues raising from this, and find the time to do a huge portion of refactoring, and this refactoring does not break the whole thing, I think it might be possible to provide such a mechanism.

In the end, I think you are better on writing your own TestEngine for the time being, even though I would love to provide you with such a mechanism.

mibutec commented 6 years ago

@bechte Mostly of all I'm missing Test Descriptions of Dynamic Tests being directly shown in my IDE after starting a test. A mechanism described by you would make it as a workaround, but letting the framework create a Test Description to overrule it by hand afterwards sounds like a featre I won't like to use.

@sbrannen @sormuras : I created locally a prototype allowing me to discover Dynamic Tests in Discovery Phase when the TestFactory method is static. Another thing looking promising: Moving the linelistener.dynamicTestRegistered(dynamicTestDescriptor); from HierarchicalTestExecutor to some stage very early in Execution Phase, so all Test Description can be shown before execution of first test.

Mostly of all both are just a bunch of lines to change with no impact on design or any architectural design desicion. So please tell me: Why is there from the beginning so much contra about the idea to provide fast feedback to developers using Dynamic Tests? Are we really talking about technical issues, or did I swim into some internal deep water or anything else?

P.S. I'm not allowed to create a branch in junit5 project (ERROR: Permission to junit-team/junit5.git denied to mibutec.). Do I need special rights or special naming conventions on branch names?

sormuras commented 6 years ago

P.S. I'm not allowed to create a branch in junit5 project (ERROR: Permission to junit-team/junit5.git denied to mibutec.). Do I need special rights or special naming conventions on branch names?

You have to fork the project first. Then, you may create as many branches as you want. From there, you may file pull requests upstream against the forked project.

sbrannen commented 6 years ago

Reopening for further team discussion...

bechte commented 6 years ago

@bechte Mostly of all I'm missing Test Descriptions of Dynamic Tests being directly shown in my IDE after starting a test. A mechanism described by you would make it as a workaround, but letting the framework create a Test Description to overrule it by hand afterwards sounds like a feature I won't like to use.

You wouldn't be able to overrule that TestDescriptor but you would be allowed to attach more children to that TestDescriptor. Replacing a TestDescriptor cannot be allowed, because it might come from another extension.

mibutec commented 6 years ago

Created:

1350 is less invasive and more powerful since it discovers even non-static @TestFactory tests early. Problem for now: Tests are discovered after execution phase for that @TestFactory method started. So if there are tests running before that method discovery starts after those are executed.

1351 introduces the idea of static @TestFactory methods. Compared to 1350 there are more code changes and some methods got ugly due to more method parameters, further refactoring is necessary. It is less powerful since non-static @TestFactory methods are not discovered early. But it seems to be a clean approach, since test discovery takes places in discovery phase.

Maybe those both are a good starting point for your team discussion.

marcphilipp commented 6 years ago

Thanks for the PRs!

Regarding #1351: IMHO executing a @TestFactory method during discovery, even when it's static, is not going to work out because these methods may have parameters to be resolved by extensions which are not registered, yet. Moreover, calling user code during discovery might cause JUnit to seem unresponsive because there's no reporting etc.

Regarding #1350: If we decide to do this, we should only do it for Collections, not for Streams, Iterators, or Iterables.

@junit-team/junit-lambda Thoughts?

sbrannen commented 6 years ago

I tend to agree with you @marcphilipp, but let's discuss it further during the next team call.

sbrannen commented 6 years ago

Team Decision: the team does not feel that the minor, aesthetic improvement proposed in these PRs would outweigh the added complexity necessary to support such a feature.

mibutec commented 6 years ago

Hey, the issue was open for almost whole easter, so maybe it will come as Easteregg :laughing:

Just for my curiosity: As far I could appraise the needed changes, there are just some code movements for 1350 necessary. What is added complexity?

sbrannen commented 6 years ago

Any introduction of new types, methods, or method parameters as well as additional behavior added to existing behavior or changes to existing behavior that are not in line with the documented, published APIs result in added complexity beyond the status quo.

mibutec commented 6 years ago

So, are we talking about "complexity" or about "work"? In case of work I can support you.

Afaik timing behavior of Dynamic Tests is not part of User Guide, so no changes necessary. @API annotated classes are not changed, either.

JavaDoc, Tests, Formatting, ... would be done by me.

sbrannen commented 6 years ago

Please read my response.

I explicitly stated "added complexity".