Deprecate APIs that use Hamcrest classes. Users should use java-hamcrest

[kcooney]

No description provided.

[kcooney]

First step for resolving #1145

[kcooney]

Note if we merge this pull, we'll need to add many methods to Assume to make it useful. Most likely, we would add methods that mirror the methods in org.junit.Assert.

[marcphilipp]

I think we should deprecate both ErrorCollector and ExpectedException completely. They are both part of hamcrest-junit.

[kcooney]

I don't think we should deprecate classes just because they exist in java-hamcest. The question is whether the classes would be useful without the deprecated methods.

I don't think we should deprecate ExpectedException until we have a Lambda-based replacement. It still will be useful without the deprecated APIs.

I'm less sure about what to do about ErrorCollector

[marcphilipp]

If we won't deprecate ExpectedException we will have to implement the functionality without using Hamcrest in 5.0, won't we?

[kcooney]

I think we can say the same about Assume. Truth solves this by having common assertion code that delegates to a "failure strategy". We could do something similar so methods in Assert have analogous methods in Assume and ExpectedException.

Do you want us to create replacement APIs first?

[marcphilipp]

Since hamcrest-junit includes ExpectedException and ErrorCollector it would be highly confusing to keep them in JUnit, wouldn't it? Hamcrest could subclass them maybe... but that does not sound very elegant either.

Just a thought: Is there a way to allow assertion frameworks (like Truth, AssertJ, or hamcrest-junit) to extend those rules without subclassing? Maybe a new interface Check<T> that would be used like ExpectedException.expectMessage(Check<String>).

interface Check<T> {
   void check(T item) throws AssertionError;
}

So, at least in Java 8, in Hamcrest this would be

thrown.expectMessage(message -> assertThat(message, containsString("foo")))

or in Truth:

thrown.expectMessage(message -> assertThat(message).contains("foo")))

[marcphilipp]

hamcrest-junit could provide a bit of syntactic sugar for it, something like

public class HamcrestCheck<T> implements Check<T> {
    private final Matcher<? super T> matcher;

    public static final HamcrestCheck<T> doesMatch(Matcher<? super T> matcher) {
        return new HamcrestCheck<>(matcher);
    }

    private HamcrestCheck(Matcher<? super T> matcher) {
        this.matcher = matcher;
    }

    public void check(T item) {
        assertThat(item, matcher);
    }
}

So this can be shortened to (even in Java < 8):

thrown.expectMessage(doesMatch(containsString("foo")));

[kcooney]

Well, the classes were copied from JUnit to hamcrest-junit so people could migrate by just changing imports. So the confusion is unavoidable. I do know projects that use these classes without Hamcrest, so I wouldn't want to remove them. If you think that having these classes have the same name is confusing, then maybe you could ask the Hamcrest team to rename their versions.

Truth already supports assumptions and checks (I believe without subclassing our classes):

assume().that(actualList).contains(3);

Perhaps we should ask the Hamcrest team if they would need a Check class.

If we add a Check class we would need to define generics of the API extremely carefully. Can a Check<Number> be used to do assertions on an Integer? How would one use a Check to check that a Number is a Long?

Edit: BTW, I don't think Java 8 users should be using the ExpectedException class at all. We should add methods to Assert, ErrorCollector and Assume that allow passing in a lambda and the expected exception type, possibly as soon as JUnit 5

[marcphilipp]

So, would Hamcrest's ExpectedException extend (i.e. subclass) JUnit's ExpectedException (and the same for ErrorCollector) as soon as JUnit 4.13 is released?

@npryce @sf105 What do you think?

[sf105]

Looking at the code, it needs two features: something that asserts against a given exception (possibly missing), and something to describe what was expected. Could you introduce an interface for these (ExpectedExceptionCheck ?) I appreciate that it's nice to turn everything into lambdas, but in this case these two features are bound together.

There's a second question about whether such a Check interface should be templated. My experience with Java generics has been so painful, that I'd avoid that. Either make it accept Object, or have a specific interface for this purpose. YMMV.

[marcphilipp]

@sf105 Thanks for joining the conversation, highly appreciated!

Doesn't something like

thrown.expectMessage(message -> assertThat(message, containsString("foo")))

already solve both problems? It asserts against the exception and describes what was expected. The only part that's missing is that the assertion was about the message of the expected exception. I guess that could be added by ExpectedException, though.

We might need to do a few spikes to compare the design possibilities...

[sf105]

@marcphilipp That's a neat idea.

[kcooney]

@marcphilipp considering that we agreed to have a lambda-friendly exception assertion API in 4.13, I think we are covered for the methods I am deprecating in ExpectedException. Anything else?

Do we need to replace these APIs before we deprecate them?

[marcphilipp]

Thanks, @kcooney! I've decided to merge this pull because we agree that we want to deprecate these APIs. Can you please add a section to the 4.13 release notes?

I've opened a new issue to continue the discussion whether we need to provide replacement APIs for methods in ExpectedException and ErrorCollector that have matcher parameters: #1179.