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

Deprecate MockitoHamcrest #1819

Merged
merged 1 commit into from Nov 18, 2019
Merged

Deprecate MockitoHamcrest #1819

merged 1 commit into from Nov 18, 2019

Conversation

@TimvdLippe
Copy link
Contributor

TimvdLippe commented Nov 6, 2019

This class was used during the migration period from Mockito 1 and
Mockito 2, but is no longer necessary. To be able to remove our
dependency on Hamcrest, we should remove MockitoHamcrest.

In response to #1817

This class was used during the migration period from Mockito 1 and
Mockito 2, but is no longer necessary. To be able to remove our
dependency on Hamcrest, we should remove MockitoHamcrest.
@TimvdLippe TimvdLippe requested review from mockitoguy and bric3 Nov 6, 2019
@TimvdLippe

This comment has been minimized.

Copy link
Contributor Author

TimvdLippe commented Nov 13, 2019

Friendly ping on this PR.

@TimvdLippe

This comment has been minimized.

Copy link
Contributor Author

TimvdLippe commented Nov 16, 2019

I will submit this PR next week if there are no objections.

@TimvdLippe

This comment has been minimized.

Copy link
Contributor Author

TimvdLippe commented Nov 18, 2019

There were no objections, so I am merging this.

@TimvdLippe TimvdLippe merged commit d4544a8 into release/3.x Nov 18, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mockitoguy

This comment has been minimized.

Copy link
Member

mockitoguy commented Nov 19, 2019

Sorry I'm late here! I suggest we don't deprecate it. The use case (as documented in the Javadoc) is the following: "Hamcrest integration is provided so that users can take advantage of existing Hamcrest matchers". That use case is still valid (perhaps the docs need to be clearer to address #1817).

For code that does not use Hamcrest, users should be following standard, non-Hamcrest argThat() API.

Hope that helps!

@TimvdLippe

This comment has been minimized.

Copy link
Contributor Author

TimvdLippe commented Nov 19, 2019

For code that does not use Hamcrest, users should be following standard, non-Hamcrest argThat() API.

The problem is that this project requires Hamcrest as compile-time dependency to be built. This is a problem for users who do not want to use Hamcrest, but still build the source code (as they can't use the pre-built jars)

If we want to keep supporting Hamcrest users with this API, I would suggest a mockito-hamcrest artifact that takes care of the integration. That way, those users who do not want to use Hamcrest don't rely on it as well.

WDYT?

@mansours

This comment has been minimized.

Copy link

mansours commented Nov 19, 2019

I'm novice to testing because I have to wear many hats (and I avoided getting into it like the plague in the past). Returning to Spring development in my career, and discovering Mockito this year changed my [integration testing] life. I rely heavily on googling stack overflow examples until it become memorized (if ever I do).

It seems like there is a much larger documented wealth of knowledge on hamcrest on stack overflow (maybe cause its been around a long time), and so its easy to find solutions to basic challenges while someone new to Mockito gets used to it.

For example hasEntry from this SO helped me today https://stackoverflow.com/questions/2580369/using-mockito-how-do-i-match-against-the-key-value-pair-of-a-map

I like TimvdLippe's suggestion because it leverages giving your users the ability to tap into that knowledge/solutions while slimming down the dependencies for the core Mockito code base.

Thanks for all your contributions in this area, and letting us stand on your shoulders.

@TimvdLippe

This comment has been minimized.

Copy link
Contributor Author

TimvdLippe commented Nov 19, 2019

Thanks for that response @mansours ! I will look into introducing a mockito-hamcrest artifact and figure out a way forward. Tomorrow I can revert this PR given the points raised by @mockitoguy and make that work.

@mockitoguy

This comment has been minimized.

Copy link
Member

mockitoguy commented Nov 19, 2019

This is a problem for users who do not want to use Hamcrest, but still build the source code (as they can't use the pre-built jars)

Can you elaborate this problem? We use "compileOnly" dependency for Hamcrest, just like we do for JUnit4 or OpenTest4j. This should not cause problems for consumers.

@TimvdLippe

This comment has been minimized.

Copy link
Contributor Author

TimvdLippe commented Nov 20, 2019

Yes we are facing the same issues with JUnit4 and OpenTest4J. JUnit4 is not an issue atm, as we are also using it, but OpenTest4J is currently an issue.

We are not able to include jars, as we require source code to be included and built (for security analyses). Thus, the inclusion of the dependencies during compile time does lead to problems when we want to build Mockito in isolation.

I am currently investigating solutions for JUnit4 and OpenTest4J and how that could be decoupled. I went ahead with this PR, as it was my understanding with Mockito 2 is that we wanted to remove our dependency of Hamcrest. Therefore, I assumed that deprecation was appropriate. Especially as there were no objections posted on this PR for almost 2 weeks, I understood that this was okay.

I will revert the PR for us to revisit, but I would like to come to a decision on our Hamcrest support and whether we want to support it or not at all. When we come to a decision, I would like to update our JavaDoc and wiki on that to clarify that for our users.

@mockitoguy

This comment has been minimized.

Copy link
Member

mockitoguy commented Nov 20, 2019

Especially as there were no objections posted on this PR for almost 2 weeks, I understood that this was okay

Of course! Thank you for waiting 2 weeks. Don't block on us.

Yes we are facing the same issues with JUnit4 and OpenTest4J. JUnit4 is not an issue atm, as we are also using it, but OpenTest4J is currently an issue.

Can you elaborate the problem? Is it a Google mono-repo use case? (I don't object reworking the artifacts - I want understanding for me and others :))

@TimvdLippe

This comment has been minimized.

Copy link
Contributor Author

TimvdLippe commented Nov 20, 2019

Can you elaborate the problem? Is it a Google mono-repo use case? (I don't object reworking the artifacts - I want understanding for me and others :))

This is for every repository that includes third_party code that we need to have the original source code for security analyses. That includes the mono repository, as well as some other repositories that we have.

Since the Hamcrest API is not type-safe (as it requires casting with Object vs T as argument type), we would like to move away from unsafe type casts. The ArgumentMatcher interface does not inherit that problem, but the older Hamcrest API does.

By having the Hamcrest-compatible API in the Mockito artifact, users can still use Mockito in combination with the Hamcrest API.

I would personally say that, while the Hamcrest API was originally useful for defining reusable matchers, the lack of type-safety does more harm than good. I have received good responses when I finished the migration of the ArgumentMatcher to be typed and it already caught bugs in our tests. I think we should promote the type-safe ArgumentMatcher and thus deprecate the old API, but happy to disagree on that part and leave Hamcrest compatibility in for example a separate artifact.

@mockitoguy

This comment has been minimized.

Copy link
Member

mockitoguy commented Nov 20, 2019

that we need to have the original source code for security analyses

So the problem is that in addition to Mockito source, you have to pull in Hamcrest source for the analysis? (I still don't feel I fully understand this use case).

By having the Hamcrest-compatible API in the Mockito artifact, users can still use Mockito in combination with the Hamcrest API.

That's a fair argument.

leave Hamcrest compatibility in for example a separate artifact.

To keep backwards compatibility we would need "mockito-core" -> "mockito-hamcrest". Are you thinking of reversing the dependency in future major version?

TimvdLippe added a commit that referenced this pull request Nov 20, 2019
This reverts commit d4544a8.
@TimvdLippe

This comment has been minimized.

Copy link
Contributor Author

TimvdLippe commented Nov 20, 2019

Revert of this PR is in ad2f352

@TimvdLippe

This comment has been minimized.

Copy link
Contributor Author

TimvdLippe commented Nov 20, 2019

So the problem is that in addition to Mockito source, you have to pull in Hamcrest source for the analysis? (I still don't feel I fully understand this use case).

Correct. Since jars can be built with any arbitrary code, we prefer to not check in a jar, as we cant be certain that it was built from the original source code. That's why we check in the original source code and build that instead.

To keep backwards compatibility we would need "mockito-core" -> "mockito-hamcrest". Are you thinking of reversing the dependency in future major version?

Yes, but since Maven does not allow a cyclic dependency we have to figure out what the possibilities are.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.