DATAES-654 - Add Junit5 support. #329

sothawo · 2019-10-03T16:22:38Z

This PR adds the setup for using JUnit 5 tests (the existing JUnit4 based test work as before).

First, normal unit tests that don't need an Elasticsearch cluster can be written straight away.
For tests that need an Elasticsearch cluster and the integration with Spring, there is a new @SpringDataElasticsearchTest annotation which combines the new SpringDataElasticsearchExtension and the SpringExtension.

The SpringDataElasticsearchExtension starts an Elasticsearch node on initialization of the first test class that uses this annotation. This cluster is used for all tests until the last one has finished - it's not recreated for every test class. This should make the tests run faster when more and more tests are switched to use this.

The sample tests (JUnit5SampleRestClientBasedTests for example) show how to set up a test class that has a correctly configured application context.

In addition to that it is possible by setting the environment variable SPRING_DATA_ELASTICSEARCH_TEST_CLUSTER_URL to not start a cluster but use an existing externally started instance:

SPRING_DATA_ELASTICSEARCH_TEST_CLUSTER_URL=http://my-elastic-host:9200 mvn -Dtest=JUnit5SampleRestClientBasedTests test

mp911de · 2019-10-03T16:27:40Z

pom.xml

@@ -242,7 +243,26 @@
 			</exclusions>
 		</dependency>

-		<dependency>
+        <dependency>
+            <groupId>org.junit.jupiter</groupId>


We need to consider JUnit 5 support from a build-perspective. We need to evaluate what's the best approach, ideally, we can import JUnit 5 dependencies in the same way as we do it with Junit 4.

that's right, but none of the parent spring-data poms provides a junit 5 version. I would have preferred to not define the dependency and the version here

Can you file a ticket against https://github.com/spring-projects/spring-data-build/?

You also do not want to manage the individual versions for JUnit 5 artifacts.

Rather, you should import the junit-bom for that.

And if you want to use JUnit Jupiter, you'll probably want to depend on the junit-jupiter aggregating artifact (similar to a Spring Boot starter POM).

fixed that (thanks @mp911de for adding that to spring-data-build)

Spring Data Build is pulling in junit-jupiter and junit-vintage-engine dependencies.

mp911de · 2019-10-03T16:36:57Z

...a/org/springframework/data/elasticsearch/junit/jupiter/SpringDataElasticsearchExtension.java

+		ClusterConnection clusterConnection = getClusterConnection(extensionContext);
+		Class<?> testClass = extensionContext.getRequiredTestClass();
+
+		AnnotationSupport.findAnnotatedFields(testClass, SpringDataElasticsearch.class).forEach(field -> {


Probably we should use only the built-in injection mechanism instead of assigning values to static fields.

This runs before the spring context is set up. When Spring is set up, this information must already be available to be able to inject it into the spring beans

Yeah, therefore it would make sense to associate the connection details with the Spring context. I'm not sure how much customization we can apply for SpringExtension. I think we would need to call ConfigurableListableBeanFactory.registerResolvableDependency(…) at some point. Maybe @sbrannen can hint us in the right direction.

I was thinking about extending SpringExtension but am not sure if this would be the right way. So I'd be glad for an hint how to get this logic of setting up the ClusterConnection only once for the test run and shutitng it down at the end of the tests coupled to Spring injection to have this info as a bean available.

Basically, we need to bridge resources provided by our Spring Data ES Extension into the Spring context so Spring beans can utilize it. We're on the same page that the connection details lifecycle is managed by our Spring Data ES Extension.

Yeah, therefore it would make sense to associate the connection details with the Spring context. I'm not sure how much customization we can apply for SpringExtension. I think we would need to call ConfigurableListableBeanFactory.registerResolvableDependency(…) at some point. Maybe @sbrannen can hint us in the right direction.

If that's what you want to do, you could implement that in a TestExecutionListener (Spring API, not JUnit Platform) like I demonstrated here: spring-projects/spring-framework#12947.

You might also want to consider implementing a custom org.springframework.test.context.ContextCustomizer.

Spring Boot's testing support relies heavily on the ContextCustomizer SPI for many of its features.

Thanks a lot, Sam. These are the APIs we were looking for.

thanks for all this information; I'll have a deeper look a t this the next week

mp911de

We need to sort out the Spring Data Build part of JUnit 5 support.

Then we should consider what tests actually require. Ideally, we can provide parameter extensions that produce the connection coordinate object, an interface that hands out the connected clients, specifically the objects we require for connecting using transport/REST/Reactive REST clients so that we can easily use these.
For configuration classes, it would also make sense to bind the connection info to e.g. some thread-local or register the connection factory as provides Spring Bean (ConfigurableListableBeanFactory.registerResolvableDependency(…)).

sbrannen · 2019-10-06T08:32:54Z

src/test/java/org/springframework/data/elasticsearch/JUnit5SampleRestClientBasedTests.java

+ */
+@SpringDataElasticsearchTest
+@ContextConfiguration(
+		classes = { JUnit5SampleRestClientBasedTests.class, ElasticsearchRestTemplateConfiguration.class })


It's not a good idea to use a test class as a pseudo @Configuration class (even it "lite bean mode").

Rather, you should use a static nested @Configuration class within the test class for such purposes, and that class can then @Import additional configuration like the ElasticsearchRestTemplateConfiguration.

sbrannen · 2019-10-06T08:35:15Z

src/test/java/org/springframework/data/elasticsearch/junit/jupiter/ClusterConnectionInfo.java

+	private boolean useSsl = false;
+	private String host;
+	private int port;
+	private Client client = null;


I'd make all the fields final.

sbrannen · 2019-10-06T08:37:32Z

...springframework/data/elasticsearch/junit/jupiter/ElasticsearchRestTemplateConfiguration.java

+@Configuration
+public class ElasticsearchRestTemplateConfiguration extends AbstractElasticsearchConfiguration {
+
+	@Autowired private ClusterConnectionInfo clusterConnectionInfo;


I wouldn't make this a required bean. If it's not present in the ApplicationContext, can't you just create a default instance for the user?

That would also remove the requirement that the user always configure a @Bean method for that if the defaults are sufficient.

sbrannen · 2019-10-06T08:39:40Z

src/test/java/org/springframework/data/elasticsearch/JUnit5SampleTransportClientBasedTests.java

+
+	// Test environment setup
+	// this will be set by the SpringDataElasticsearchExtension, must be static
+	@SpringDataElasticsearch static private ClusterConnectionInfo clusterConnectionInfo;


Why does this have to be static?

If you absolutely need it at the class level, have you considered using @TestInstance(PER_CLASS) semantics?

mp911de · 2019-10-14T09:29:19Z

Related: spring-projects/spring-data-build#906

mp911de · 2019-10-14T10:24:35Z

Spring Data Build includes now junit-jupiter and the vintage engine. We can remove dependency-related changes from pom.xml in this PR.

sothawo · 2019-10-15T17:27:35Z

I adaapted the pom; will look after the other points when the other PR regarding the Mappers is done, that one is more important

mp911de

We can drop JUnit dependencies entirely from the POM as Spring Data Build pulls these already. We need to address the issue about how to inject cluster connection details in a proper way into the context and then we're good to merge this PR.

mp911de · 2019-10-16T07:03:46Z

pom.xml

@@ -242,7 +243,26 @@
 			</exclusions>
 		</dependency>

-		<dependency>
+        <dependency>
+            <groupId>org.junit.jupiter</groupId>


Spring Data Build is pulling in junit-jupiter and junit-vintage-engine dependencies.

sothawo · 2019-10-17T19:04:33Z

My problem is, that when implementing a TestExecutionListener or a ContextCustomizer I do not have access to the ClusterConnection object that is stored in the JUnit5 ExtensionContext. TestExecutionListener gets a Spring TestContext which is independent from the JUnit5 ExtensionContext.

I though about extending SpringExtension and putting the ClusterConnection into the TestContext, but the method getTestContextManage() is private and so I have no access from a derived class.

sothawo · 2019-10-17T19:41:02Z

I now implemented this using a ThreadLocal setting it up in our extension and then retrieving it in the configuration class.

mp911de

We're getting there and this PR makes good progress. I added a few remarks to improve the integration. We may want to remove the dependency to SpringRunner from @SpringDataElasticsearchTest to make the Spring aspect optional for running integration tests without a context.

mp911de · 2019-10-18T09:16:04Z

...t/java/org/springframework/data/elasticsearch/junit/jupiter/SpringDataElasticsearchTest.java

+@Target(ElementType.TYPE)
+@ExtendWith(SpringDataElasticsearchExtension.class)
+@ExtendWith(SpringExtension.class)
+public @interface SpringDataElasticsearchTest {


I'm not sure whether we need this, but how about renaming this annotation to @IntegrationTest? It probably makes sense to remove @Extend(SpringExtension) here and introduce it in Spring-specific tests. The reason is that SpringDataElasticsearchExtension should handle primarily the Elasticsearch connectivity aspect. The integration with Spring is a secondary concern. See also comments on SpringDataElasticsearchExtension.

The reason I created this annotation was to combine the two extensions in the right order, so that SpringDataElasticsearchExtension runs before SpringExtension. One can still use the extensions on their own wit @ExtendWithlike I did in the newJUnit5ClusterConnectionTests`test

Then let's shoot for @SpringIntegrationTest and @IntegrationTest annotations for easier consumption in the tests.

mp911de · 2019-10-18T09:21:31Z

...a/org/springframework/data/elasticsearch/junit/jupiter/SpringDataElasticsearchExtension.java

+ *
+ * @author Peter-Josef Meisch
+ */
+public class SpringDataElasticsearchExtension implements BeforeAllCallback {


It would make sense to implement also ParameterResolver here. This is, to give non-Spring tests the chance to connect to Elasticsearch using a client so tests can inject ClusterConnection into the test method or a setup method.

Furthermore, we need to address the integration with Spring. Given the ordering aspect, we might want to make this class also implement ContextCustomizerFactory to customize the Spring context before it bootstraps (requires META-INF/spring.factories). We could register a resolvable dependency with

@Override public ContextCustomizer createContextCustomizer(Class<?> testClass, List<ContextConfigurationAttributes> configAttributes) { return this::customizeContext; } private void customizeContext(ConfigurableApplicationContext context, MergedContextConfiguration mergedConfig) { context.getBeanFactory() .registerResolvableDependency(ClusterConnectionInfo.class, ClusterConnection .getClusterConnectionInfo()); }

so that configuration classes can declare a @Autowired ClusterConnectionInfo and so we connect both pieces.

I wasn't able to achieve the same using a TestExecutionListener as the listener invocation seems too late for our purposes.

implemented this; I just didn't see the last connecting step from my implementation yesterday and then context customizing ;-) Now Autowiring works and parameter resolution as well

mp911de · 2019-10-18T09:22:43Z

...org/springframework/data/elasticsearch/junit/jupiter/ElasticsearchTemplateConfiguration.java

+public class ElasticsearchTemplateConfiguration extends ElasticsearchConfigurationSupport {
+
+	@Bean
+	public Client elasticsearchClient() {


We can inject ClusterConnectionInfo as a method parameter. Alternatively, a @Autowired ClusterConnectionInfo field.

mp911de · 2019-10-18T09:23:08Z

src/test/java/org/springframework/data/elasticsearch/junit/jupiter/ClusterConnectionInfo.java

+ * @author Peter-Josef Meisch
+ */
+public final class ClusterConnectionInfo {
+	final private boolean useSsl;


Nit: private final instead of final private.

mp911de · 2019-10-18T09:23:24Z

src/test/java/org/springframework/data/elasticsearch/junit/jupiter/ClusterConnectionInfo.java

+public final class ClusterConnectionInfo {
+	final private boolean useSsl;
+	final private String host;
+	final private int port;


What type of port is this? HTTP or Transport Client?

mp911de · 2019-10-18T09:26:28Z

src/test/java/org/springframework/data/elasticsearch/junit/jupiter/ClusterConnection.java

+ *
+ * @author Peter-Josef Meisch
+ */
+class ClusterConnection implements ExtensionContext.Store.CloseableResource {


It would make sense to store both information, the endpoint and the handle to a cluster runner (Node client, Test containers) in the Jupiter context for separation of concerns.

The ClusterConnection is stored in the ExtensionContext. Or do you mean having separate objects for the different connection types? I think, we will remove the client part when removing transport client stuff, so we then will only need the host/port/ssl part. Even when adding support for TestContainers (optional test depenedency?), the connection to the ES in a TestContainer will be defined by the URL

See comment below in the actual extension.

mp911de

Great work. I left a few comments about high-level cleanups that help us going forward with the tests to reduce pitfalls regarding threading.

Feel free to merge this PR after having a look.

mp911de · 2019-10-20T11:44:55Z

src/test/java/org/springframework/data/elasticsearch/junit/jupiter/IntegrationTest.java

+import org.springframework.test.context.junit.jupiter.SpringExtension;
+
+/**
+ * Wraps the {@link SpringExtension}.


Nit: SpringDataElasticsearchExtension

mp911de · 2019-10-20T11:46:07Z

...a/org/springframework/data/elasticsearch/junit/jupiter/SpringDataElasticsearchExtension.java

+	@Override
+	public Object resolveParameter(ParameterContext parameterContext, ExtensionContext extensionContext)
+			throws ParameterResolutionException {
+		return ClusterConnection.getClusterConnectionInfo();


Storing getClusterConnectionInfo within the ExtensionContext.store would remove the ThreadLocal dependency from the lookup.

mp911de · 2019-10-20T11:46:31Z

src/test/java/org/springframework/data/elasticsearch/junit/jupiter/ClusterConnection.java

+ *
+ * @author Peter-Josef Meisch
+ */
+class ClusterConnection implements ExtensionContext.Store.CloseableResource {


See comment below in the actual extension.

mp911de · 2019-10-20T11:47:16Z

src/test/java/org/springframework/data/elasticsearch/junit/jupiter/ClusterConnectionInfo.java

+ *
+ * @author Peter-Josef Meisch
+ */
+public final class ClusterConnectionInfo {


Nit: Let's add a bit of proper Javadoc to this class and the builder.

mp911de · 2019-10-20T11:51:19Z

...a/org/springframework/data/elasticsearch/junit/jupiter/SpringDataElasticsearchExtension.java

+	public void beforeAll(ExtensionContext extensionContext) throws Exception {
+		initLock.lock();
+		try {
+			ExtensionContext rootContext = extensionContext.getRoot();


In this method we can guarantee single-threaded behavior, therefore I'd suggest looking up ClusterConnection and storing it in the ExtensionContext.store, too. With migrating resolveParameter(…) to use the extension store, we no longer depend on threading for the parameter lookup. The only integration that needs to worry about Thread affinity is how we pass ClusterConnection into the Spring context.

sothawo requested review from mp911de and xhaggi October 3, 2019 16:22

mp911de reviewed Oct 3, 2019

View reviewed changes

mp911de requested changes Oct 3, 2019

View reviewed changes

sbrannen reviewed Oct 6, 2019

View reviewed changes

sothawo force-pushed the DATAES-654_Add-JUnit5-support branch from a7c5d15 to c769faf Compare October 12, 2019 13:51

sothawo force-pushed the DATAES-654_Add-JUnit5-support branch 2 times, most recently from 986acde to dc4d8e5 Compare October 16, 2019 04:47

mp911de reviewed Oct 16, 2019

View reviewed changes

sothawo force-pushed the DATAES-654_Add-JUnit5-support branch 2 times, most recently from 35596a5 to a433ade Compare October 17, 2019 19:37

sothawo requested a review from mp911de October 17, 2019 19:41

mp911de requested changes Oct 18, 2019

View reviewed changes

sothawo force-pushed the DATAES-654_Add-JUnit5-support branch 2 times, most recently from 8c1dfc2 to 9d354a4 Compare October 18, 2019 13:39

mp911de approved these changes Oct 20, 2019

View reviewed changes

sothawo force-pushed the DATAES-654_Add-JUnit5-support branch from 9d354a4 to a437e6f Compare October 20, 2019 17:56

DATAES-654 - Add Junit5 support.

7211281

sothawo force-pushed the DATAES-654_Add-JUnit5-support branch from a437e6f to 7211281 Compare October 20, 2019 18:04

sothawo merged commit 58920af into spring-projects:master Oct 20, 2019

sothawo deleted the DATAES-654_Add-JUnit5-support branch October 20, 2019 18:36

spring-projects-issues mentioned this pull request Dec 31, 2020

Add Junit 5 support [DATAES-654] #1230

Closed

DATAES-654 - Add Junit5 support. #329

DATAES-654 - Add Junit5 support. #329

Conversation

sothawo commented Oct 3, 2019

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

mp911de 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

mp911de commented Oct 14, 2019

mp911de commented Oct 14, 2019

sothawo commented Oct 15, 2019

mp911de left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sothawo commented Oct 17, 2019

sothawo commented Oct 17, 2019

mp911de 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

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

mp911de 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