Skip to content

Only add hamcrest-junit dependency when using assertThat or assumeThat#577

Merged
timtebeek merged 8 commits intoopenrewrite:mainfrom
FieteO:575-junit5-remove-hamcrest-junit
Aug 17, 2024
Merged

Only add hamcrest-junit dependency when using assertThat or assumeThat#577
timtebeek merged 8 commits intoopenrewrite:mainfrom
FieteO:575-junit5-remove-hamcrest-junit

Conversation

@FieteO
Copy link
Copy Markdown
Contributor

@FieteO FieteO commented Aug 14, 2024

What's changed?

Remove the hamcrest-junit dependency when running the JUnit4to5Migration recipe.

What's your motivation?

The dependency is not needed in the Junit 5 context (AFAIK)

@timtebeek
Copy link
Copy Markdown
Member

timtebeek commented Aug 14, 2024

How does this interact with this recipe a little further up/down? As we seem to add that explicitly in a JUnit 5 upgrade

rewrite-testing-frameworks/src/main/resources/META-INF/rewrite/junit5.yml

Line 92 in b7d3b84

- org.openrewrite.java.testing.hamcrest.AddHamcrestIfUsed

rewrite-testing-frameworks/src/main/resources/META-INF/rewrite/junit5.yml

Lines 166 to 172 in b7d3b84

- org.openrewrite.java.dependencies.AddDependency:
groupId: org.hamcrest
artifactId: hamcrest-junit
version: 2.x
scope: test
onlyIfUsing: org.junit.Assume
acceptTransitive: true

What makes that the dependency is not or no longer needed in your case?

@FieteO
Copy link
Copy Markdown
Contributor Author

FieteO commented Aug 14, 2024 •
edited
Loading

Hmm, you are right. Probably some more investigation is needed as to when the dependency is needed/when it can be removed.
The hamcrest-junit seems to have been the result of decoupling the Hamcrest portions in Junit 4 into a separate dependency.
This dependency hasn't received an update since 9 years and it's hard to say what use it serves. The PR that added it to the junit5.yml was titled Migrate JUnit 4 Assume#assumeThat()tohamcrest-junit`` to fix issue #327 .

As far as I can tell, Junit 5 also has support for Assumptions.
Maybe @knutwannheden can comment on this?

@timtebeek timtebeek marked this pull request as draft August 14, 2024 17:45
github-actions[bot]
github-actions Bot reviewed Aug 17, 2024
View reviewed changes
Comment thread src/test/java/org/openrewrite/java/testing/hamcrest/AddHamcrestJUnitDependencyTest.java
Comment thread src/test/java/org/openrewrite/java/testing/hamcrest/AddHamcrestJUnitDependencyTest.java Outdated
github-actions[bot]
github-actions Bot reviewed Aug 17, 2024
View reviewed changes
Comment thread src/test/java/org/openrewrite/java/testing/hamcrest/AddHamcrestJUnitDependencyTest.java Outdated
@timtebeek timtebeek changed the title Remove hamcrest-junit dependency when migrating from JUnit 4 to 5 Only add hamcrest-junit dependency when using assertThat or assumeThat Aug 17, 2024
@timtebeek timtebeek added the bug Something isn't working label Aug 17, 2024
@timtebeek timtebeek self-requested a review August 17, 2024 11:25
@timtebeek timtebeek marked this pull request as ready for review August 17, 2024 11:25
@timtebeek
Copy link
Copy Markdown
Member

timtebeek commented Aug 17, 2024

This was a fun one @FieteO ; thanks for raising this for visibility. Turns out we were a little too eager adding that hamcrest-junit dependency on any use of org.junit.Assume, as opposed to only when using assertThat or assumeThat. Solved now by creating a new scanning recipe that looks at methods in use and only if found adds the dependency.

timtebeek
timtebeek approved these changes Aug 17, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ended up removing the declarative AddDependency that was there before, and implementing a scanning recipe that's a little more selective. Thanks again for calling out this issue with over eager dependency addition!

@timtebeek timtebeek merged commit b885d12 into openrewrite:main Aug 17, 2024
@FieteO
Copy link
Copy Markdown
Contributor Author

FieteO commented Aug 18, 2024

Great, that there indeed was something to improve here. Thanks @timtebeek for working on this.

@timtebeek timtebeek mentioned this pull request Aug 22, 2024
Closed
@timtebeek timtebeek linked an issue Aug 22, 2024 that may be closed by this pull request
Remove hamcrest-junit dependency in Junit4to5Migration recipe #575
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@timtebeek timtebeek timtebeek approved these changes

Assignees

@FieteO FieteO

Labels

bug Something isn't working

Projects

OpenRewrite
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove hamcrest-junit dependency in Junit4to5Migration recipe

2 participants

@FieteO @timtebeek