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

Resurrect MatcherAssume.assumeThat #322

Open
hakanai opened this issue Dec 1, 2020 · 7 comments
Open

Resurrect MatcherAssume.assumeThat #322

hakanai opened this issue Dec 1, 2020 · 7 comments

Comments

@hakanai
Copy link

hakanai commented Dec 1, 2020

hamcrest-junit used to have MatcherAssume class with static method assumeThat:
https://www.javadoc.io/doc/org.hamcrest/hamcrest-junit/latest/org/hamcrest/junit/MatcherAssume.html#assumeThat(T,%20org.hamcrest.Matcher)

This appears to have been accidentally(?) omitted when relocating org.hamcrest.junit.* to org.hamcrest.

JUnit subsequently have deprecated Assert.assertThat on the basis that Hamcrest's MatcherAssert has it.

This now leaves us in a weird situation where we import JUnit's Assume.assumeThat while importing Hamcrest's MatcherAsserts.assertThat.

(People wanting to use JUnit 5 would probably be in an even worse situation, because JUnit 5 doesn't have assumeThat in any form, as far as I can tell, and neither does Hamcrest, leading me to wonder what JUnit 5 users have been doing for assumeThat all this time...)

It would make sense to me if MatcherAssume were resurrected so that both can be imported from Hamcrest.

@hakanai
Copy link
Author

hakanai commented Feb 18, 2021

I'm willing to take this one on if I can get some guidance on which direction to go.

I tried to create a MatcherAssume class and implement it but I couldn't throw either AssumptionViolatedException (JUnit4) or TestAbortedException (opentest4j, used by JUnit5) because neither was on the classpath.

@peterdemaeyer
Copy link
Contributor

peterdemaeyer commented Feb 21, 2021

To get the record straight on JUnit 5: it has Assumptions.assumeTrue, assumeFalse, aligned with JUnit's Assertions.assertTrue, assertFalse.

@peterdemaeyer
Copy link
Contributor

There seems to be an integration problem here.
AFAICT, Hamcrest is designed to be implementation-independent.
By lack of a common way of failing assumptions, implementing assumptions in Hamcrest would require it to become implementation-specific: JUnit 4, JUnit 5, or maybe yet something else in another test framework.
OpenTest4J seems to be an attempt at creating some common ground between testing frameworks, and at least JUnit 5 follows it.
It seems to me we need at least an additional dependency on OpenTest4J.
That project builds with Java 11 though, and Hamcrest is still on 1.7, so I'm not sure that will work out.

Yet another alternative is to somehow optionally allow integration with multiple alternative testing frameworks.
I see there is already a hamcrest-integration subproject, but in its current form doesn't look like an out-of-the-box fit either...

In a nutshell: I'm just sparring ideas here, but I don't have a solution either that doesn't require marrying Hamcrest to a particular test framework.

@hakanai
Copy link
Author

hakanai commented Feb 22, 2021

Yeah, certainly hamcrest used to have integration modules like hamcrest-junit, but eventually it was all rolled back into one again. opentest4j seemed to be the only thing to integrate with, but it's still one more dependency so it seems like people would have to be happy with it to proceed anyway.

I wonder what other frameworks like assert4j are doing with junit for assumptions.

peterdemaeyer added a commit to peterdemaeyer/JavaHamcrest that referenced this issue Feb 23, 2021
@peterdemaeyer
Copy link
Contributor

peterdemaeyer commented Feb 23, 2021

While I was investigating this a bit further, I took my own stab at it.

For JUnit 5, I basically settled on the same conclusion: we need a dependency on OpenTest4J.
Ideally, speaking in Maven terms, that would be an "optional" dependency.
In Gradle terms (which I was not familiar with until just now) that apparently translates to "feature variants", but in order to be able to use that feature we would need to upgrade Gradle to 6.8.3.
I briefly tried that, bumped into non-obvious problems of having to replace 'osgi' plugin with 'bnd' plugin, and gave up for now, deciding that it was beyond the scope of this issue.

For JUnit 4, I concluded that we should not try to fix it, and instead accept JUnit 4's own assertThat methods, which already use the proper declarative syntax anyway.
JUnit 4 depends on Hamcrest, so it would introduce a circular dependency if we would depend Hamcrest back on JUnit 4, which would be "wrong".

I briefly investigated if we should stick it maybe in the hamcrest-integration subproject, but quickly conluded that was an old and deprecated project that is best left alone or even deleted, alongside with hamcrest-core and hamcrest-library, but that exercise is again beyond the scope of this issue.

In any case, while investigating I ended up with a fix and unit tests so created a PR for it.

@hakanai
Copy link
Author

hakanai commented Jun 23, 2021

One wonders what happened to hamcrest-junit - it seemed to exist specifically to solve the issue of dependencies by decoupling the two projects entirely, and Hamcrest could then have just introduced a hamcrest-junit5 or whatever to deal with JUnit 5.

@CC007
Copy link

CC007 commented Aug 8, 2021

I just ran into the issue while trying to use an assumption with junit5 while using SMOG to generate the matchers. If it's needed I'm all for the existence of a hamcrest-junit5 library, but this lack of assumtions in hamcrest is a serious issue.

From what I saw, assumptions throw a TestAbortedException instead of an AssertionFailedError. This TestAbortedException is indeed a opentest4j class. Maybe a construction could be made where the Exception class for a failed assumption is configurable (and maybe by default uses TestAbortedException if it finds opentest4j on the classpath).

peterdemaeyer added a commit to peterdemaeyer/JavaHamcrest that referenced this issue Dec 25, 2021
peterdemaeyer added a commit to peterdemaeyer/JavaHamcrest that referenced this issue Dec 26, 2021
peterdemaeyer added a commit to peterdemaeyer/JavaHamcrest that referenced this issue Dec 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants