hamcrest / JavaHamcrest

Java (and original) version of Hamcrest
http://hamcrest.org/
BSD 3-Clause "New" or "Revised" License
2.11k stars 379 forks source link

Map hasEntry combined with collection matcher seems to fail incorrrectly #284

Open jamesmudd opened 4 years ago

jamesmudd commented 4 years ago

Here is some example code showing what I think is a bug

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.hasEntry;
import static org.hamcrest.Matchers.not;

import java.util.HashMap;
import java.util.List;
import java.util.Map;

class Scratch {

    public static void main(String[] args) {
        Map<String, Object> map = new HashMap<>();
        List<String> value = List.of("value1", "value2");
        map.put("key", value);
        assertThat(map, hasEntry("key", not(empty())));

    }
}

The assert fails with the output

Exception in thread "main" java.lang.AssertionError: 
Expected: map containing ["key"-><not an empty collection>]
     but: map was [<key=[value1, value2]>]
    at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:18)

But key is mapped to a "not empty collection"? So I think it should pass.

jamesmudd commented 4 years ago

Note on Java 11 with Hamcrest 2.1

nhojpatrick commented 4 years ago

Can you test with hamcrest v2.2

nhojpatrick commented 4 years ago

Your saying Java 11, are you aware if it works on another Java version?

jamesmudd commented 4 years ago

Its exactly the same issue on Java 11 with Hamcrest v2.2 and on Java 8 with Hamcrest v2.2

nhojpatrick commented 4 years ago

Can you try the following and see if it works for you hasEntry(equalTo("key"), not(empty())).

The method hasEntry(K, V) assumes your giving literal Key and Value parameters. So it takes your not(empty()) and assumes it's a value, and instantly wraps it as equalTo(not(empty())).

The other method hasEntry(Matcher<K>, Matcher<V>) works for me as it takes matchers for both Key and Value. We don't have a method that takes the combination for hasEntry(K, Matcher<V>) currently which we would need for your original test.

jamesmudd commented 4 years ago

Yes that works. Ok so what would be needed for this is 2 extra signatures for hasEntry

If you think that would be an acceptable solution I can look at making a PR? I think it would be a nice feature.

nhojpatrick commented 4 years ago

Yes that works. Ok so what would be needed for this is 2 extra signatures for hasEntry

  • hasEntry(K key, Matcher<? super V> valueMatcher)
  • hasEntry(Matcher<? super K> keyMatcher, V value)

If you think that would be an acceptable solution I can look at making a PR? I think it would be a nice feature.

It would make sense to overload org.hamcrest.collection.IsMapContaining.hasEntry to have those signatures you have listed. I'm sure additional methods would get approved.

sf105 commented 4 years ago

Please be careful with this. Things get hard to predict with this level of overloading. It's easy to end up trying to do equals() on a matcher object. Perhaps a better option would be both matchers?