JavaHamcrest icon indicating copy to clipboard operation
JavaHamcrest copied to clipboard

Map hasEntry combined with collection matcher seems to fail incorrrectly

Open jamesmudd opened this issue 5 years ago • 8 comments

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 avatar Mar 04 '20 11:03 jamesmudd

Note on Java 11 with Hamcrest 2.1

jamesmudd avatar Mar 04 '20 11:03 jamesmudd

Can you test with hamcrest v2.2

nhojpatrick avatar Mar 04 '20 11:03 nhojpatrick

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

nhojpatrick avatar Mar 04 '20 11:03 nhojpatrick

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

jamesmudd avatar Mar 04 '20 12:03 jamesmudd

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.

nhojpatrick avatar Mar 04 '20 14:03 nhojpatrick

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.

jamesmudd avatar Mar 04 '20 14:03 jamesmudd

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.

nhojpatrick avatar Mar 04 '20 19:03 nhojpatrick

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?

sf105 avatar Mar 04 '20 22:03 sf105