Issue433 adapters for junit4 rules #526

mmerdes · 2016-09-28T12:41:03Z

Overview

This PR provides an initial implementation for #433.

Both, field and method rules are supported.
Implementations of org.junit.rules.Verifier and org.junit.rules.ExternalResource are supported. Currently, this supported is activated by an explicit @ExtendWith.
This might be replaced by a classpath-based activation (expensive) or better by some sort of meta-annotation, e.g. @EnableJUnit4RuleSupport.
Currently, there is no warning issued if an unsupported rule type is used.

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

bechte · 2016-10-01T15:34:45Z

First of all, thanks @mmerdes for the effort. Do I understand your code right, that it will require a compile time dependency to the JUnit4 core to support the JUnit4 @Rule annotation and the proposed rules as of #433 (+comments)? If we go along the route, this should be at least only a runtime dependency. .o(This would only be a minor thing to tackle)

Reading the tests for your implementation is giving me a good look & feel, how this thing will be used in the future. It comes along with advantages for test migration, i.e. the basic rules would require no change to the code. Still I feel, that we open the door for expectation far to wide. Allowing the basic rules will grow expectations for the other rules to follow. We might end up loosing the control of the test execution the same way as JUnit4 with @Rules.

As @kcooney already mentioned on #169 (comment), most rules only require to participate one or many of the three phases "setup", "teardown", and "afterEach". Our extension model allows exactly this (and that's the way you've implemented your adapters). The only thing that is missing is, to allow people to register an extension at the field level. If we had @ExtendWith for fields, one could simple replace the @Rule annotation from JUnit4 with @ExtendWith annotation from JUnit5 during the migration. The tests have to been transformed anyway to reference the JUnit5 version of the @Test annotation, therefore, it wouldn't be much hassle to also replace the @Rule.

Now, as we already discussed, @ExtendWith on the field level doesn't read well and it somehow behaves different (it is not registering an extension for its own level and all tests below the test hierarchy, but it adds another extension to the class level). Therefore, I would vote for using a different annotation that communicates its intend better, and provides the functionality needed, i.e. registering extensions on the field level: @UseExtension or @WithExtension

The annotation will have no values as the Type of the field should resolve to an Extension class. With this mechanism in place a migration of the rules included in #433 (+comments) would be possible like this:

// code before migration
@Rule
public TemporaryFolder folder = new TemporaryFolder();

@Rule
public ExternalResource resource1 = new ExternalResource() {
   // ...
}

// code after migration
@UseExtension
public TemporaryFolder folder = new TemporaryFolder();

@UseExtension
public ExternalResource resource1 = new ExternalResource() {
   // ...
}

Note these 3 things on the code:

@Rule will be replaced by @UseExtension (or @WithExtension, you-name-it)
TemporaryFolder will not be resolved to the JUnit4 Rule org.junit.rules.TemporaryFolder but to a new class that we have to provide which basically does the same thing.
ExternalResource will not be resolved to the JUnit4 Rule org.junit.rules.ExternalResource but to a new class that we have to provide which basically does the same thing.

Still, very simple and the migration could be done by a script, searching & replacing the parts. This way, I would have a much better feeling. If the wants me to provide this as an alternative approach, I would be happy to do so. But please let us take very careful steps with this topic. I have a really bad feeling about opening the gates to modify the test execution lifecycle.

mmerdes · 2016-10-06T14:57:34Z

@bechte
Thanks for your extended review.
Yes, a runtime dependency to JUnit 4 is of course required. As stated in the description of this PR the exact mechanics are not yet fixed. I outlined tow options and stated my preference. So far so good.
Concerning the main part of your comment:
If I understand you correctly, you are not comfortable with the requirements of this issue. However, these requirements have been fixed long ago in the respective issue and specifically included the verbatim use of the JUnit 4 @Rule syntax without the need for a script transformation etc.
As an additional approach field-based extensions are discussed here in #497.
I think we all agree that multiple migration options might be suitable for different companies/situations.

limited support for JUnit 4 rules of type org.junit.rules.ExternalResource optional dependency of the Jupiter engine on JUnit 4 #433 (cherry picked from commit a20fd0d)

add tests for the initial rule support. #433

new integration test with 2 rules and after invocation verification #433

#433

Refactor ExternalResourceSupport to prepare for rule-annotated methods and other TestRules #433

Support rule-annotated methods and make the afterAll-check fail the build if appropriate. #433

Refactor ExternalResourceSupport to handle the method and field cases in separate (sub)classes.

The ExternalResourceSupport is now composed of the two separately implemented support classes for method and field support, respectively. Includes a test to verify the relative order of both cases. #433

Includes a test case that verifies inherited rule-annotated fields and methods are handled correctly

Refactoring to prepare the generalization from ExternalResource to other suitable subtypes of TestRule

#433

…from JUnit 4 This test would fail correctly if commented in so serves as documentation only. #433

#433

marcphilipp

Overall I think this looks like it's almost good to go. I just found a few minor things. 👍

marcphilipp · 2016-11-19T10:47:53Z

.../src/main/java/org/junit/jupiter/migrationsupport/rules/adapter/AbstractTestRuleAdapter.java

+
+	private void failIfAdapteeClassIsNotAssignableFromTargetClass() {
+		if (!this.adapteeClass.isAssignableFrom(this.target.getClass()))
+			throw new IllegalStateException(this.adapteeClass + " is not assignable from " + this.target.getClass());


How about using Preconditions?

marcphilipp · 2016-11-19T10:49:02Z

.../src/main/java/org/junit/jupiter/migrationsupport/rules/adapter/AbstractTestRuleAdapter.java

+			ReflectionUtils.invokeMethod(method, target);
+		}
+		catch (NoSuchMethodException | SecurityException exception) {
+			LOG.warning(exception.getMessage());


I think this should throw an exception instead of logging it. Otherwise, it would only be visible in a log that something was silently ignored.

marcphilipp · 2016-11-19T10:52:28Z

...-support/src/main/java/org/junit/jupiter/migrationsupport/rules/ExternalResourceSupport.java

+	@Override
+	public void beforeEach(TestExtensionContext context) throws Exception {
+		this.fieldSupport.beforeEach(context);
+		this.methodSupport.beforeEach(context);


This should be methods first, then fields to maximize compatibility with Vintage.

marcphilipp · 2016-11-19T10:53:32Z

...ion-support/src/main/java/org/junit/jupiter/migrationsupport/rules/TestRuleFieldSupport.java

+			Collectors.toList());
+	}
+
+	// TODO: decide whether this should be promoted to AnnotationUtils


I think it should but not necessarily in this pull request. Can you please create an issue for it and delete this comment?

marcphilipp · 2016-11-19T10:54:53Z

...upport/src/main/java/org/junit/jupiter/migrationsupport/rules/member/RuleAnnotatedField.java

+			this.testRuleInstance = (TestRule) testRuleField.get(testInstance);
+		}
+		catch (IllegalAccessException exception) {
+			LOG.warning(exception.getMessage());


marcphilipp · 2016-11-19T10:55:29Z

...pport/src/main/java/org/junit/jupiter/migrationsupport/rules/member/RuleAnnotatedMethod.java

+public class RuleAnnotatedMethod extends AbstractRuleAnnotatedMember {
+
+	public RuleAnnotatedMethod(Object testInstance, Method member) {
+		//no args


Issues: #433, #169

marcphilipp · 2016-11-19T14:55:56Z

.../src/main/java/org/junit/jupiter/migrationsupport/rules/adapter/AbstractTestRuleAdapter.java

+			return ReflectionUtils.invokeMethod(method, target, arguments);
+		}
+		catch (NoSuchMethodException | SecurityException exception) {
+			LOG.warning(exception.getMessage());


I think this should throw an exception instead of logging it. Otherwise, it would only be visible in a log that something was silently ignored.

marcphilipp · 2016-11-24T21:06:54Z

in progress

marcphilipp · 2016-11-25T16:55:19Z

This pull request has been manually rebased onto master and merged.

@mmerdes Thanks for all your hard work! 👍

sbrannen · 2016-11-29T00:52:54Z

+1: thanks, @mmerdes! 👍

I'm looking forward to hearing what the community has to say about this migration support.

mmerdes force-pushed the issue433-adapters-for-junit4-rules branch 4 times, most recently from 74ac611 to ec4b3de Compare November 4, 2016 12:30

mmerdes force-pushed the issue433-adapters-for-junit4-rules branch from 59620e2 to 12e4e07 Compare November 18, 2016 16:18

Matthias Merdes and others added 23 commits November 18, 2016 17:23

Support rules of type ExternalResource

0b53a34

limited support for JUnit 4 rules of type org.junit.rules.ExternalResource optional dependency of the Jupiter engine on JUnit 4 #433 (cherry picked from commit a20fd0d)

Support rules of type ExternalResource

4bd1278

add tests for the initial rule support. #433

Support rules of type ExternalResource

2486378

new integration test with 2 rules and after invocation verification #433

Support multiple rules on fields

fa9c3da

#433

Refactor ExternalResourceSupport

dcfaff0

Refactor ExternalResourceSupport to prepare for rule-annotated methods and other TestRules #433

Support rule-annotated methods

08f4839

Support rule-annotated methods and make the afterAll-check fail the build if appropriate. #433

Refactor to separate cases

b31e1bc

Refactor ExternalResourceSupport to handle the method and field cases in separate (sub)classes.

Compose support of method and field support

c8761c9

The ExternalResourceSupport is now composed of the two separately implemented support classes for method and field support, respectively. Includes a test to verify the relative order of both cases. #433

Support inherited rule-annotated fields and methods

0099c36

Includes a test case that verifies inherited rule-annotated fields and methods are handled correctly

Generalize to other subtypes of TestRule

e65d97d

Refactoring to prepare the generalization from ExternalResource to other suitable subtypes of TestRule

Support Verifier implementations

c9799f0

#433

Cleanup and simplify implementation

8fee0a6

#433

Add integration test for Verifier-rules using ErrorCollector example …

4972b54

…from JUnit 4 This test would fail correctly if commented in so serves as documentation only. #433

Improve package structure

7d77762

#433

Move rule support to dedicated module

c51083d

#433

Add @API-annotations to all classes

cee2b61

#433

Add JavaDoc to both new extensions

9db45fd

#433

Add readme file for the new module

5711992

#433

Introduce meta-annotation for turning on rule-support

f6022aa

#433

Reduce redundancy concerning failures in @AfterAll-methods

24e1b25

#433

Fix Javadoc

e13de8a

#433

Add launcher-based test for meta-annotation, tests for wrong extensions

2c23b1c

#433

Add protection against mismatching target and adaptee classes

6066569

#433

Polishing

7b25a68

#433

mmerdes force-pushed the issue433-adapters-for-junit4-rules branch from 12e4e07 to 7b25a68 Compare November 18, 2016 16:47

Fix problem with test-compile dependency

b1942ea

#433

marcphilipp requested changes Nov 19, 2016

View reviewed changes

Add support for ExpectedException

031463b

Issues: #433, #169

marcphilipp reviewed Nov 19, 2016

View reviewed changes

marcphilipp added this to the 5.0 M3 milestone Nov 19, 2016

marcphilipp self-assigned this Nov 22, 2016

marcphilipp added 5 commits November 24, 2016 21:17

Use Preconditions instead of throwing IllegalStateException

97ad3a2

Remove System.out.println calls from tests

967d318

Use assertTrue() instead of assert keyword

558f1fb

Throw JUnitException for error during method lookup

13467ff

Change rule execution order for fields/methods to be like in Vintage

1e2a95f

marcphilipp added the status: in progress label Nov 24, 2016

marcphilipp removed the status: in progress label Nov 25, 2016

marcphilipp closed this Nov 25, 2016

marcphilipp deleted the issue433-adapters-for-junit4-rules branch November 27, 2016 09:56

marcphilipp added the mentioned in release notes label Nov 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue433 adapters for junit4 rules #526

Issue433 adapters for junit4 rules #526

mmerdes commented Sep 28, 2016 •

edited by marcphilipp

Loading

bechte commented Oct 1, 2016 •

edited

Loading

mmerdes commented Oct 6, 2016

marcphilipp left a comment

marcphilipp Nov 19, 2016

marcphilipp Nov 19, 2016

marcphilipp Nov 19, 2016

marcphilipp Nov 19, 2016

marcphilipp Nov 19, 2016

marcphilipp Nov 19, 2016

marcphilipp Nov 19, 2016

marcphilipp commented Nov 24, 2016

marcphilipp commented Nov 25, 2016

sbrannen commented Nov 29, 2016

Issue433 adapters for junit4 rules #526

Issue433 adapters for junit4 rules #526

Conversation

mmerdes commented Sep 28, 2016 • edited by marcphilipp Loading

Overview

bechte commented Oct 1, 2016 • edited Loading

mmerdes commented Oct 6, 2016

marcphilipp left a comment

Choose a reason for hiding this comment

marcphilipp Nov 19, 2016

Choose a reason for hiding this comment

marcphilipp Nov 19, 2016

Choose a reason for hiding this comment

marcphilipp Nov 19, 2016

Choose a reason for hiding this comment

marcphilipp Nov 19, 2016

Choose a reason for hiding this comment

marcphilipp Nov 19, 2016

Choose a reason for hiding this comment

marcphilipp Nov 19, 2016

Choose a reason for hiding this comment

marcphilipp Nov 19, 2016

Choose a reason for hiding this comment

marcphilipp commented Nov 24, 2016

marcphilipp commented Nov 25, 2016

sbrannen commented Nov 29, 2016

mmerdes commented Sep 28, 2016 •

edited by marcphilipp

Loading

bechte commented Oct 1, 2016 •

edited

Loading