-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 Factory Extension #530
Conversation
…that identifies potential test methods
This feature is implemented much like TestFactoryMethodResolver and TestFactoryTestDescriptor. So far it only supports test factory extension methods.
In case anybody checked out this branch: I force-pushed a couple of changes 😊 to keep the history somewhat clean. |
Now that M3 is out the door, I want to give this PR a gentle push. It would be great if someone could look into it. 😄 |
We will, sorry for not doing so, yet! However, we're really short on time right now so it might take a few days. |
It was obviously out of scope for M3 so it's reasonable that it didn't get much attention. And I didn't want to hurry anyone, take all the time you need. I just wanted this to be on the radar. |
@nicolaiparlog Did the "@testtemplate" framework already superseed the goal you wanted to achieve here? If not, do you have the time to rebase your PR? The topic is on my radar now and a compiling proof-of-concept would speed up my learning pace. |
This PR is a shot at implementing #371. It creates a new extension point TestFactoryExtension. As a proof of concept, the machinery behind the
@TestFactory
annotation was moved to the new extension point. All tests pass with hardly any changes.Details
TestFactoryExtensions implement
Stream<DynamicTest> createForMethod(TestExtensionContext)
and can be applied to methods (see caveat below). The extension machinery (inTestFactoryExtensionResolver
andTestFactoryExtensionMethodTestDescriptor
) identifies methods that were extended with TestFactoryExtension, instantiates the extension implementation, callscreateForMethod
to create the dynamic tests, and finally runs the tests.The new
TestFactoryAnnotationExtension
(automatically applied to all@TestFactory
methods) implementsTestFactoryExtension
by reflectively calling the extended method. It then returns theStream
the method produced.This means the tasks (and code) from the now removed
TestFactoryTestDescriptor
were largely split between the newTestFactoryExtensionMethodTestDescriptor
andTestFactoryAnnotationExtension
.Trouble With
ExtensionRegistry
There seem to be no tests verifying that
@TestFactory
methods can be called with parameters. That's a lucky coincidence because it doesn't work any more...Resolving method parameters is part of applying extensions to the called method. To this end the now removed
TestFactoryTestDescriptor
called itsexecutableInvoker
with the extension registry it got from the engine execution context:This is no longer possible because the task of invoking the test method was moved to the
TestFactoryExtension
, which is part of junit-jupiter-api. The extension registry lives in junit-jupiter-engine and can not be referenced in and hence not passed to the extension.Even if it could, it would be doubtful whether that's a good idea. The registry is only really usable together with the rest of JUnit's extension machinery. This is no problem for the
TestFactoryAnnotationExtension
because it lives in the same project (or so I thought until I realized that precisely this dependencyTestFactoryAnnotationExtension ~> ExecutableInvoker
caused a dependency cycle). But third-party extensions are not in that lucky situation. If they ever were to call methods to which extensions should be applied, they'd not only need the extension registry but rather a simplifiedExecutableInvoker
that already knows the relevantTestExtensionContext
andExtensionRegistry
instances - partial application so to speak.If you think this is worth discussing I'm happy to open an issue for it.
Coming back to the dependency cycle
TestFactoryAnnotationExtension ~> ExecutableInvoker
...It creates a dependency from the packageextension
toexecution
, which creates a cycle becauseexecution
already depends onextension
because the latter contains theExecutionRegistry
. One fix for this would be to generally allow extensions to use features fromexecution
and move the registry to the latter. While this might seem wrong at first, I saw thatExtensionValuesStore
lives there as well, maybe providing precedence.Methods vs Containers
The current implementation only supports extending methods. But much of the code that's needed to extend whole containers is already present. What's missing is a
TestFactoryExtensionClassTestDescriptor
, which represents the extended container and callsTestFactoryExtension::createForContainer
to create its tests.Naming
Most type names contain the word "Extension", which allows an easy distinction of types that belong to the new extension point and those that belong to
@TestFactory
. This made implementation easier and should also ease discussions. But I'd propose to drop the "Extension" in all type names to make them more succinct. That being said, it is unclear howTestFactoryExtension
and@TestFactory
should then be named.I hereby agree to the terms of the JUnit Contributor License Agreement.