-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Fix ResultMatchers#hasFailureContaining #1292
Fix ResultMatchers#hasFailureContaining #1292
Conversation
Also, introduce hamcrest-library as a dependency. hamcrest-library contains more Matchers.
It should not match when the given PrintableResult has no failures.
@@ -110,7 +110,7 @@ | |||
<dependencies> | |||
<dependency> | |||
<groupId>org.hamcrest</groupId> | |||
<artifactId>hamcrest-core</artifactId> | |||
<artifactId>hamcrest-library</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should change this dependency.
@junit-team/junit-committers Does anyone know why |
My guess is that In any case, it's too late to move it now |
That's true, unfortunately. @alb-i986 Can you try to solve this without using |
also improve unit tests
Done, but why's that? |
I don't want to force regular users and IDEs like Eclipse (who bundle JUnit) to jump through hoops just so we can use |
I'm not sure what you mean by "jump through hoops", but ok, as it turned out I could do without it. But like at least we could add hamcrest-library as a test dependency for ourselves. But I'm not sure if that's easy to configure. |
Yeah, that would make sense and should be easy to configure. |
/** | ||
* Matches if the number of failures matches {@code countMatcher} | ||
*/ | ||
public static Matcher<PrintableResult> failureCount(final Matcher<Integer> countMatcher) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why we need an overload that takes a Matcher<Integer>
. Can you give an example of where this would be more useful than the version that takes an int
?
Also, if we add this, shouldn't the parameter have a type of Matcher<? super Integer>
(see http://hamcrest.org/JavaHamcrest/javadoc/1.3/org/hamcrest/Matchers.html#arrayWithSize(org.hamcrest.Matcher) for one example in Hamcrest)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing failureCountIs(final int count)
only allows to do an equals match, whereas taking a Matcher of Integer allows clients to do any kind of integer matching, eg. <, >, >=, <=.
It came from the original implementation of this PR 9f5dfd8
But since I later changed implementation (69e8a8a), it's not needed for this PR. I've removed it and I'll open a new PR for that, as I think it still makes sense on its own.
Please let me know your thoughts
@alb-i986 would you please update the release notes? |
ResultMatchers.hasFailureContaining() should not match when the given PrintableResult has no failures.
It should not match when the given PrintableResult has no failures.
Please note that this change replaces the dependency hamcrest-core with hamcrest-library. I'm not sure if you are happy with that. If you are, we probably also need to update another couple of lines in the pom, see e.g. https://github.com/junit-team/junit4/blob/master/pom.xml#L308