Introduce TestInstanceFactory extension API #1443

kdubb · 2018-06-04T03:41:53Z

Overview

TestInstanceFactory is a new extension API that allows custom creation of test instance objects.

It must be applied at the class level.
If multiple TestInstanceFactory extensions are found to be registered on a test class an error is thrown.
Supports @Nested classes, ensuring that only one factory is registered on each level.

Issue: #672

I hereby agree to the terms of the JUnit Contributor License Agreement.

Definition of Done

There are no TODOs left in the code
Method preconditions are checked and documented in the method's Javadoc
Coding conventions (e.g. for logging) have been followed
Change is covered by automated tests
Public API has Javadoc and @API annotations
Change is documented in the User Guide and Release Notes
All continuous integration builds pass

kdubb · 2018-06-04T03:49:54Z

@sbrannen This extension solves issues for test instances that need to be vended in a special manner; specifically this greatly simplifies testing using CDI for Weld Junit. Would love to see this or version of this implemented very soon so I thought I'd take a stab at it. Let me know your thoughts and suggested changes, if any.

codecov · 2018-06-04T03:50:41Z

Codecov Report

Merging #1443 into master will decrease coverage by 0.03%.
The diff coverage is 88.57%.

@@             Coverage Diff             @@
##             master   #1443      +/-   ##
===========================================
- Coverage     92.03%     92%   -0.04%     
- Complexity     3416    3428      +12     
===========================================
  Files           316     318       +2     
  Lines          8173    8204      +31     
  Branches        696     701       +5     
===========================================
+ Hits           7522    7548      +26     
- Misses          485     489       +4     
- Partials        166     167       +1

Impacted Files	Coverage Δ	Complexity Δ
...iter/api/extension/TestInstantiationException.java	`0% <0%> (ø)`	`0 <0> (?)`
...r/engine/descriptor/NestedClassTestDescriptor.java	`100% <100%> (ø)`	`5 <0> (ø)`	⬇️
...jupiter/engine/descriptor/ClassTestDescriptor.java	`99.28% <100%> (+0.12%)`	`57 <10> (+9)`	⬆️
...e/execution/DefaultTestInstanceFactoryContext.java	`100% <100%> (ø)`	`3 <3> (?)`
...jupiter/engine/execution/ExtensionValuesStore.java	`87.14% <0%> (-1.43%)`	`21% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38e149f...c7241da. Read the comment docs.

sormuras

Thanks for the PR, looks good so far and instantiation of nested classes is covered. Exception-handling needs some love, though.

sormuras · 2018-06-04T05:54:34Z

junit-jupiter-api/src/main/java/org/junit/jupiter/api/extension/TestInstanceFactory.java

+ * <p>Consult the documentation in {@link Extension} for details on
+ * constructor requirements.
+ *
+ * @since 5.0


sormuras · 2018-06-04T05:55:24Z

junit-jupiter-api/src/main/java/org/junit/jupiter/api/extension/TestInstanceFactory.java

+ * @see #instantiateTestClass(Class, ExtensionContext)
+ * @see #instantiateNestedTestClass(Class, Object, ExtensionContext)
+ */
+@API(status = STABLE, since = "5.0")


Let's start with status = EXPERIMENTAL and since = "5.3".

sormuras · 2018-06-04T05:57:18Z

junit-jupiter-api/src/main/java/org/junit/jupiter/api/extension/TestInstanceFactory.java

+	 * @param context the current extension context; never {@code null}
+	 * @return The required test instance; never {@code null}
+	 */
+	Object instantiateTestClass(Class<?> testClass, ExtensionContext context) throws Exception;


Create an dedicated exception type that extends JUnitException. TestInstanceCreationException?

sormuras · 2018-06-04T05:57:39Z

junit-jupiter-api/src/main/java/org/junit/jupiter/api/extension/TestInstanceFactory.java

+	 * @return The required test instance; never {@code null}
+	 */
+	Object instantiateNestedTestClass(Class<?> testClass, Object outerInstance, ExtensionContext context)
+			throws Exception;


Use same dedicated exception type as above.

sormuras · 2018-06-04T06:02:26Z

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/ClassTestDescriptor.java

+				"Too many TestInstanceFactory extensions [%s] registered on test class: %s", factoryNames,
+				testClass.getSimpleName());
+
+			throw new JUnitException(errorMessage);


At least throw an instance of ExtensionContextException.

sormuras · 2018-06-04T06:03:37Z

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/ClassTestDescriptor.java

+		if (factory == null) {
+			return invokeTestInstanceConstructor(outerInstance, registry, extensionContext);
+		}
+		else {


else not needed here.

sormuras · 2018-06-04T06:06:38Z

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/ClassTestDescriptor.java

+					return factory.instantiateTestClass(this.testClass, extensionContext);
+				}
+			}
+			catch (RuntimeException exception) {


Both catch are too generic. And please use BlacklistedExceptions.rethrowIfBlacklisted(cause);

sormuras · 2018-06-04T06:11:05Z

...upiter-engine/src/test/java/org/junit/jupiter/engine/extension/TestInstanceFactoryTests.java

+/**
+ * Integration tests that verify support for {@link TestInstanceFactory}.
+ *
+ * @since 5.0


kdubb · 2018-06-04T19:51:49Z

@sormuras Your changes have been pushed

sbrannen · 2018-06-05T13:20:36Z

documentation/src/docs/asciidoc/user-guide/extensions.adoc

@@ -241,6 +241,21 @@ Examples:
 - `org.example.MyCondition`: deactivates the condition whose FQCN is exactly
  `org.example.MyCondition`.

+[[extensions-test-instance-factories]]
+=== Test Instance Factories
+`{TestInstanceFactory}` defines the API for `Extensions` that wish to _provide new_


A corresponding link must also be added to /documentation/src/docs/asciidoc/link-attributes.adoc.

sbrannen · 2018-06-05T13:21:53Z

documentation/src/docs/asciidoc/user-guide/extensions.adoc

+[WARNING]
+====
+Registering multiple extensions that implement `{TestInstanceFactory}` will result in an
+exception being thrown for all tests in the test class. It is the users responsibility to


... or test class hierarchy

users --> user's

sbrannen · 2018-06-05T13:23:18Z

...jupiter-api/src/main/java/org/junit/jupiter/api/extension/TestInstanceCreationException.java

+ * @since 5.3
+ */
+@API(status = EXPERIMENTAL, since = "5.3")
+public class TestInstanceCreationException extends JUnitException {


I think TestInstantiationException would be a more suitable name

sbrannen · 2018-06-05T13:25:01Z

junit-jupiter-api/src/main/java/org/junit/jupiter/api/extension/TestInstanceFactory.java

+ *
+ * <p>Extensions that implement {@code TestInstanceFactory} must be
+ * registered at the class level.
+ *


The class-level JavaDoc for TestInstanceFactory should also mention that only one such extension may be registered per test class or test class hierarchy, analogous to the documentation in the User Guide.

sbrannen · 2018-06-05T13:28:02Z

junit-jupiter-api/src/main/java/org/junit/jupiter/api/extension/TestInstanceFactory.java

+	 * only attempt to create the required test instance.
+	 *
+	 * @param testClass the test class to instantiate or otherwise obtain
+	 * @param outerInstance instance of outer test class (if any)


If the method is solely dedicated to instantiating a nested test class (i.e., a non-static nested class), it then does not make sense for the outerInstance to ever be null. Thus the documentation must state ;never {@code null}.

sbrannen · 2018-06-05T13:28:47Z

junit-jupiter-api/src/main/java/org/junit/jupiter/api/extension/TestInstanceFactory.java

+	 * @return The required test instance; never {@code null}
+	 * @throws TestInstanceCreationException when an error occurs with the invocation of a factory
+	 */
+	Object instantiateNestedTestClass(Class<?> testClass, Object outerInstance, ExtensionContext context)


Another option would be to have a single such method in this new API and make the outerInstance an Optional.

sbrannen · 2018-06-05T13:34:16Z

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/ClassTestDescriptor.java

+
+		if (parentRegistry != null) {
+			List<TestInstanceFactory> parentFactories = parentRegistry.getExtensions(TestInstanceFactory.class);
+			factories.removeAll(parentFactories);


If we are going to ignore factories registered in parent contexts -- if and only if there is a locally registered factory -- we then need to explicitly document that behavior.

I have clarified the behavior in both the javadoc and user guide.

sbrannen · 2018-06-05T13:35:50Z

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/ClassTestDescriptor.java

+		}
+		catch (Throwable exception) {
+			BlacklistedExceptions.rethrowIfBlacklisted(exception);
+			throw exception;


This combination does not make sense.

What are you trying to achieve?

I misunderstood what @sormuras had suggested and exactly wash rethrowIfBlacklisted was to be used for. I have just removed this handling entirely to let the exception propagate as normal.

sbrannen · 2018-06-05T13:38:52Z

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/ClassTestDescriptor.java

+
+		try {
+			if (outerInstance != null) {
+				return factory.instantiateNestedTestClass(this.testClass, outerInstance, extensionContext);


This approach makes it impossible for such a constructor to accept arguments provided by a ParameterResolver -- for example, TestInfo, TestReporter, mocks from Mockito, beans from Spring, etc.

We need to decide if we want this new extension API to suffer from this restriction.

I do not believe this is a restriction that impacts the majority of use cases for factories. I believe factories are specifically for when customized construction is required by external libraries; in which case arguments from JUnit 5 sources are probably not going to be required. Also, the current extension API seems to cover these situations well and using a factory doesn't negate post-processing or any other extension.

kdubb · 2018-06-05T17:55:39Z

@sbrannen I believe the changes you've requested have been pushed. Let me know if anything is still outstanding.

sbrannen · 2018-06-06T10:02:43Z

documentation/src/docs/asciidoc/link-attributes.adoc

@@ -65,6 +65,7 @@ endif::[]
 :RepetitionInfo:                    {javadoc-root}/org/junit/jupiter/api/RepetitionInfo.html[RepetitionInfo]
 :TestExecutionExceptionHandler:     {javadoc-root}/org/junit/jupiter/api/extension/TestExecutionExceptionHandler.html[TestExecutionExceptionHandler]
 :TestInfo:                          {javadoc-root}/org/junit/jupiter/api/TestInfo.html[TestInfo]
+:TestInstanceFactory:               {javadoc-root}/org/junit/jupiter/api/extension/TestInstanceFactory.html[TestInstanceFactoryh]


Remove trailing "h" in link name.

TestInstanceFactoryh --> TestInstanceFactory

sbrannen · 2018-06-06T10:05:51Z

My comments:

This approach makes it impossible for such a constructor to accept arguments provided by a ParameterResolver -- for example, TestInfo, TestReporter, mocks from Mockito, beans from Spring, etc.

We need to decide if we want this new extension API to suffer from this restriction.

Response from @kdubb:

I do not believe this is a restriction that impacts the majority of use cases for factories. I believe factories are specifically for when customized construction is required by external libraries; in which case arguments from JUnit 5 sources are probably not going to be required. Also, the current extension API seems to cover these situations well and using a factory doesn't negate post-processing or any other extension.

@junit-team/junit-lambda, the team needs to reach a consensus on this before continuing with this PR.

sbrannen · 2018-06-06T10:06:42Z

@kdubb,

I believe the changes you've requested have been pushed.

Thanks

Let me know if anything is still outstanding.

I noticed an extra "h", but I think that's all for now.

marcphilipp · 2018-06-08T10:48:59Z

junit-jupiter-api/src/main/java/org/junit/jupiter/api/extension/TestInstanceFactory.java

+	 * @return the required test instance; never {@code null}
+	 * @throws TestInstantiationException when an error occurs with the invocation of a factory
+	 */
+	Object instantiateTestClass(Class<?> testClass, Optional<Object> outerInstance, ExtensionContext context)


Since the team already foresees that we might eventually need to add a callback into the existing parameter resolution mechanism, I think it would be better to use a parameter object, e.g. TestInstanceFactoryContext that groups testClass and outerInstance. This would allow us to later add a method to the context etc. (see ParameterResolver/ParameterContext for an example of this approach).

kdubb · 2018-06-19T18:06:33Z

@marcphilipp @sbrannen Not sure if you guys saw but I committed the changes @marcphilipp requested. Is there anything else that needs to happen? Would have love to have this in M1 if possible.

marcphilipp

I think this PR is almost ready to be merged. I'm missing a test for @TestInstance(PER_CLASS). Could you please add that?

@nested

`TestInstanceFactory` is a new extension point that allows custom creation of test instance objects. * It must be applied at the class level * If multiple `TestInstanceFactories` are found to be registered on a test class an error is thrown * Supports @nested classes, ensuring that only one factory is registered on each level

* Declare `TestInstanceFactory` methods to throw `TestInstanceCreationException` * Mark new API as `EXPERIMENTAL` * Mark relevant files as `@since 5.3`

* Combine API methods into one; no compelling reason for separating top-level and nested factory methods. * Rename TestInstanceCreationException -> TestInstantiationException * Clarify documenation & javadoc on nested factory handling * Remove incorrect exception “filtering”

* Added `TestInstanceFactoryContext` to encapsulate the information a `TestInstanceFactory` _may_ need to instantiate a test class * Changed signature of `instantiateTestClass` to use `TestInstanceFactoryContext` instead of individual parameters

kdubb · 2018-06-29T01:53:01Z

@marcphilipp Test for PER_CLASS was added and rebased onto latest master

sbrannen · 2018-06-30T14:04:06Z

Thanks for adding the additional tests, @kdubb.

sbrannen · 2018-06-30T16:39:58Z

This has been merged into master in 5b4c642 and polished in subsequent commits.

sormuras requested changes Jun 4, 2018

View reviewed changes

marcphilipp added this to the 5.3 M1 milestone Jun 4, 2018

marcphilipp requested review from marcphilipp and sbrannen June 4, 2018 10:43

kdubb force-pushed the test-instance-factory-extensions branch from 1baecf1 to 583377c Compare June 4, 2018 21:28

sbrannen requested changes Jun 5, 2018

View reviewed changes

sbrannen changed the title ~~Add TestInstanceFactory extension point~~ Introduce TestInstanceFactory extension API Jun 5, 2018

sormuras approved these changes Jun 5, 2018

View reviewed changes

sbrannen approved these changes Jun 6, 2018

View reviewed changes

sbrannen requested changes Jun 6, 2018

View reviewed changes

marcphilipp added the status: team discussion label Jun 6, 2018

marcphilipp requested changes Jun 8, 2018

View reviewed changes

sormuras modified the milestones: 5.3 M1, 5.3 M2 Jun 15, 2018

marcphilipp assigned sbrannen Jun 22, 2018

ghost assigned marcphilipp Jun 24, 2018

ghost added the status: in progress label Jun 24, 2018

marcphilipp requested changes Jun 24, 2018

View reviewed changes

marcphilipp removed their assignment Jun 24, 2018

kdubb force-pushed the test-instance-factory-extensions branch from 837d29a to 52b168a Compare June 29, 2018 01:37

kdubb added 3 commits June 28, 2018 18:50

Requested fixes

54c99c4

* Declare `TestInstanceFactory` methods to throw `TestInstanceCreationException` * Mark new API as `EXPERIMENTAL` * Mark relevant files as `@since 5.3`

Update documentation to include TestInstanceFactory

a95abc4

kdubb and others added 5 commits June 28, 2018 18:51

Add factory extension news to release notes

99fd321

Fix typo in link attributes

3aa5620

Polish Javadoc

89b8ead

kdubb force-pushed the test-instance-factory-extensions branch from 52b168a to 21e2788 Compare June 29, 2018 01:52

Add test coverage for factories and PER_CLASS

c7241da

kdubb force-pushed the test-instance-factory-extensions branch from 21e2788 to c7241da Compare June 29, 2018 02:02

sbrannen approved these changes Jun 30, 2018

View reviewed changes

sbrannen added theme: extensions component: Jupiter and removed status: team discussion labels Jun 30, 2018

sbrannen closed this Jun 30, 2018

ghost removed the status: in progress label Jun 30, 2018

seanf mentioned this pull request Aug 28, 2018

Question: JUnit 5 Support? cdi-unit/cdi-unit#103

Closed

Introduce TestInstanceFactory extension API #1443

Introduce TestInstanceFactory extension API #1443

Conversation

kdubb commented Jun 4, 2018 • edited by sbrannen Loading

Overview

Definition of Done

kdubb commented Jun 4, 2018 • edited Loading

codecov bot commented Jun 4, 2018 • edited Loading

Codecov Report

sormuras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdubb commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdubb commented Jun 5, 2018

Choose a reason for hiding this comment

sbrannen commented Jun 6, 2018

sbrannen commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdubb commented Jun 19, 2018 • edited Loading

marcphilipp left a comment

Choose a reason for hiding this comment

kdubb commented Jun 29, 2018

sbrannen commented Jun 30, 2018

sbrannen commented Jun 30, 2018

kdubb commented Jun 4, 2018 •

edited by sbrannen

Loading

kdubb commented Jun 4, 2018 •

edited

Loading

codecov bot commented Jun 4, 2018 •

edited

Loading

kdubb commented Jun 19, 2018 •

edited

Loading