Skip to content
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

Upgrade to Hamcrest 2.1 #15555

Closed
ankurpathak opened this issue Dec 22, 2018 · 20 comments
Closed

Upgrade to Hamcrest 2.1 #15555

ankurpathak opened this issue Dec 22, 2018 · 20 comments
Assignees
Labels
theme: testing Issues related to testing type: dependency-upgrade A dependency upgrade
Milestone

Comments

@ankurpathak
Copy link

ankurpathak commented Dec 22, 2018

Spring Boot Starter Test still uses old Hamcrest 1.3. Now Hamcrest 2.1 released after 3 years and have many new matchers, so it make sense to use it in Spring Boot Starter Test, Please check this url
for more details:
hamcrest/JavaHamcrest#224

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 22, 2018
@snicoll snicoll changed the title Add Support For Hamcrest 2.1 in Spring Boot Starter Test Upgrade to hamcrest 2.1 Dec 24, 2018
@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Dec 24, 2018
@wilkinsona
Copy link
Member

so it make sense to use it in Spring Boot Starter Test

Thanks for the suggestion, but I'm not sure that it makes sense, at least not without considering other dependencies.

JUnit 4.12 depends on Hamcrest 1.3. We'd either need to be certain that it is compatible with 2.1 or consider the Hamcrest upgrade alongside a switch to JUnit 5 (version 5 dropped JUnit's Hamcrest dependency).

@wilkinsona wilkinsona added the theme: testing Issues related to testing label Jan 10, 2019
@wilkinsona
Copy link
Member

Let's consider this in 2.2.x alongside #14736.

@wilkinsona wilkinsona added status: blocked An issue that's blocked on an external project change type: dependency-upgrade A dependency upgrade and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Jan 10, 2019
@wilkinsona wilkinsona added this to the 2.2.x milestone Jan 10, 2019
@wilkinsona wilkinsona added status: blocked An issue that's blocked on an external project change and removed status: blocked An issue that's blocked on an external project change labels May 17, 2019
@wilkinsona
Copy link
Member

This may still be blocked. While we have upgraded to JUnit 5 in spring-boot-starter-test, to allow users to gently migrate to it, we provide JUnit's Vintage Engine and the JUnit 4 API in the starter. Unfortunately, this means that Hamcrest 1.3 is still pulled in.

@sbrannen Do you know if JUnit 4, when being used "just" as an API on top of the Vintage Engine, requires Hamcrest 1.3, or can we exclude it and upgrade to 2.1?

@sbrannen
Copy link
Member

sbrannen commented May 22, 2019

At a first glance, it did not appear to be possible due to packaging changes in Hamcrest from 1.3 to 2.x; however, after some internal discussions with @marcphilipp and a bit of investigative work, it appears that it might be possible by following the Upgrading from Hamcrest 1.x guidelines.

For example, I have the JUnit 4 build working with the following dependencies:

	<dependencies>
		<dependency>
			<groupId>org.hamcrest</groupId>
			<artifactId>hamcrest-core</artifactId>
			<version>1.3</version>
		</dependency>
		<dependency>
			<groupId>org.hamcrest</groupId>
			<artifactId>hamcrest</artifactId>
			<version>2.1</version>
			<scope>test</scope>
		</dependency>
	</dependencies>

Though I still need to check if I can get something similar working with a project using the JUnit Vintage TestEngine on the JUnit Platform.

@sbrannen
Copy link
Member

OK, I've gotten the following to work.

  1. Create a vanilla Maven-based app using start.spring.io with Spring Boot 2.2 M3.
  2. Change the dependencies to the following.
<dependencies>
	<dependency>
		<groupId>org.springframework.boot</groupId>
		<artifactId>spring-boot-starter</artifactId>
	</dependency>

	<dependency>
		<groupId>org.springframework.boot</groupId>
		<artifactId>spring-boot-starter-test</artifactId>
		<scope>test</scope>
		<exclusions>
			<exclusion>
				<groupId>org.hamcrest</groupId>
				<artifactId>hamcrest-core</artifactId>
			</exclusion>
			<exclusion>
				<groupId>org.hamcrest</groupId>
				<artifactId>hamcrest-library</artifactId>
			</exclusion>
		</exclusions>
	</dependency>
	<dependency>
		<groupId>org.hamcrest</groupId>
		<artifactId>hamcrest</artifactId>
		<version>2.1</version>
		<scope>test</scope>
	</dependency>
</dependencies>
  1. Run the generated contextLoads() test method on the JUnit Vintage TestEngine (e.g., by running the test using the "JUnit 5" support in your IDE).

Modifying the generated test class as follows also works.

import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.test.context.junit4.SpringRunner;

@RunWith(SpringRunner.class)
@SpringBootTest
public class Hamcrest21ApplicationTests {

	@Test
	public void contextLoads() {
		assertThat(2 * 3, is(6));
	}

}

@wilkinsona, let me know if that's sufficient for you and whether or not it works for your purposes.

@wilkinsona
Copy link
Member

wilkinsona commented May 22, 2019

Thanks, @sbrannen. That sounds hopeful on the JUnit side of things. Before I go looking, do you have any information to hand about the packaging changes in Hamcrest 2.1? Our goal in Boot 2.2 is to migrate people seamlessly onto JUnit 5 via the vintage engine. If an upgrade to Hamcrest 2.1 would require changes to a user's tests if they're using Hamcrest, we may want to hold off on moving to 2.1.

@sbrannen
Copy link
Member

When I said "packaging", I meant the actual JARs produced, not changes to Java package names. Sorry if that was misleading.

AFAIK, Hamcrest 2.1 should be compatible with Hamcrest 1.3 except that the internal @Factory annotation was removed (which hopefully no Spring Boot users depended on), and a few method signatures were changed with regard to generics.

The latter change likely only applies to users that wrote custom matchers, and resulting compiler errors can be fixed by replacing <T> with <? extends T> in generic type declarations for the affected methods (at least for the changes I encountered).

Regarding the published artifacts, you'll find a summary here, and if you want full details you'll need to read the discussions in hamcrest/JavaHamcrest#224.

@sbrannen
Copy link
Member

If an upgrade to Hamcrest 2.1 would require changes to a user's tests if they're using Hamcrest, we may want to hold off on moving to 2.1.

Just to reiterate on the above points...

  1. Hamcrest 2.1 appears to be fully binary compatible with Hamcrest 1.3.
  2. The new hamcrest artifact contains everything previously in hamcrest-core and hamcrest-library (except the @Factory annotation).
  3. AFAIK, users were not supposed to use the @Factory annotation. So although that is gone, I assume it won't affect typical users.
  4. Users with custom Matcher implementations may need to make slight modifications to generic signatures in order to compile against Hamcrest 2.1, but code previously compiled against Hamcrest 1.3 should run against Hamcrest 2.1 without recompilation.

@wilkinsona
Copy link
Member

Thanks very much, @sbrannen. Sounds like we can proceed with this. The impact on users is likely to be nil. If they have custom matchers the modifications that are needed are minimal enough to meet our requirement for minor Boot releases to be easy to adopt.

@wilkinsona wilkinsona removed the status: blocked An issue that's blocked on an external project change label May 22, 2019
@wilkinsona wilkinsona self-assigned this May 22, 2019
@wilkinsona wilkinsona modified the milestones: 2.2.x, 2.2.0.M4 May 22, 2019
@spencergibb
Copy link
Member

FYI the hamcrest API is not totally backward compatible. It broke a test in spring-cloud-kubernetes https://jenkins.spring.io/view/Spring%20Cloud/view/CI.MASTER/job/spring-cloud-kubernetes-master-ci/7157/console

spring-cloud/spring-cloud-kubernetes@59a55fd

@sbrannen
Copy link
Member

Ouch!

16:23:42 [ERROR] /opt/jenkins/data/workspace/spring-cloud-kubernetes-master-ci/spring-cloud-kubernetes-integration-tests/discovery/tests/src/test/java/org/springframework/cloud/kubernetes/it/ServicesIT.java:[44,62] constructor StringContains in class org.hamcrest.core.StringContains cannot be applied to given types;
16:23:42 [ERROR]   required: boolean,java.lang.String
16:23:42 [ERROR]   found: java.lang.String
16:23:42 [ERROR]   reason: actual and formal argument lists differ in length

That's very unfortunate. 😞

@tumbarumba, I understood that Hamcrest 2.1 was intended to be API compatible with Hamcrest 1.3.

Are you aware of any other such breaking changes (other than changes to method signatures related to generics as I mentioned earlier)?

Also, are there are plans to publish a point release (e.g., Hamcrest 2.1.1) that reinstates original APIs such as the StringContains constructor mentioned in the above compilation error?

@sbrannen
Copy link
Member

sbrannen commented May 22, 2019

On a side note, @spencergibb, why don't you switch to the StringContains.containsString(...) factory method instead of the constructor for StringContains (potentially with a static import)?

.then().statusCode(200).body(containsString("service-a") {

@spencergibb
Copy link
Member

@sbrannen yeah, I'm sure the test could change. I didn't write it but came from the community.

@wilkinsona
Copy link
Member

Oh dear. Thanks for letting us know, @spencergibb. Sounds like we need to consider reverting this.

@wilkinsona wilkinsona reopened this May 22, 2019
@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label May 22, 2019
@wilkinsona
Copy link
Member

Judging by this commit, it looks like StringEndsWith and StringStartsWith are affected too.

I wonder how many people will be calling the constructors (which are not backwards compatible) directly rather than using the static factory methods (which are backwards compatible). My hunch is that use of the latter will be much more common than the former but that is nothing more than a hunch.

@philwebb
Copy link
Member

I've raised hamcrest/JavaHamcrest#259 to ask them to restore the old constructors.

@philwebb
Copy link
Member

I'm of the opinion that we should probably upgrade anyway. The fix is relatively simple and hopefully Hamcrest will provide a patch sometime in the future.

@wilkinsona
Copy link
Member

Ok, let's stick with this for now. We can see how things pan out prior to 2.2's GA.

@wilkinsona wilkinsona removed the for: team-attention An issue we'd like other members of the team to review label May 23, 2019
@tumbarumba
Copy link

I see that this now closed, so this may be moot, but I've just committed a fix to restore the original 1.3 compatible constructors: hamcrest/JavaHamcrest#260.

As suggested by @sbrannen above, it's unusual for the constructors to be used directly - it's more usual for the static builder methods to be used. But, given that the constructors are public, they should be kept backwards compatible.

@sbrannen
Copy link
Member

Awesome! Thanks for providing the fix so quickly, @tumbarumba. 👏

@wilkinsona wilkinsona changed the title Upgrade to hamcrest 2.1 Upgrade to Hamcrest 2.1 Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: testing Issues related to testing type: dependency-upgrade A dependency upgrade
Projects
None yet
Development

No branches or pull requests

8 participants