From a474ac143488eec90bf6a6644b55b3250c4398fd Mon Sep 17 00:00:00 2001 From: peteg Date: Mon, 7 Jan 2019 03:43:14 -0800 Subject: [PATCH] Handle exceptions from Correspondence.compare in the Fuzzy Truth assertions implemented directly in MultimapSubject. They were already handled in containsExactly(EntriesIn) because those methods delegate to IterableSubject which handled them. However, this CL does add some tests for those (because at some point we plan to implement them directly in MultimapSubject to improve the failure messages, and it would be good to have the test cases in place before we do that). This completes the work for Correspondence.compare. There's still work to do for exceptions from Correspondence.formatDiff, and from the function used to pair elements for diffing. RELNOTES=All Fuzzy Truth assertions now handle exceptions thrown by Correspondence.compare (see the javadoc of that method for details). ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=228137650 --- .../google/common/truth/Correspondence.java | 1 - .../google/common/truth/MultimapSubject.java | 110 ++++++--- .../common/truth/MultimapSubjectTest.java | 230 ++++++++++++++++-- 3 files changed, 296 insertions(+), 45 deletions(-) diff --git a/core/src/main/java/com/google/common/truth/Correspondence.java b/core/src/main/java/com/google/common/truth/Correspondence.java index 0538ccf9b..1537a217f 100644 --- a/core/src/main/java/com/google/common/truth/Correspondence.java +++ b/core/src/main/java/com/google/common/truth/Correspondence.java @@ -175,7 +175,6 @@ public String toString() { * returned false. (Note that, in the latter case at least, it is likely that the test author's * intention was not for the test to pass with these values.) */ - // TODO(b/119038411): Ensure that all callers in Truth handle exceptions sensibly // TODO(b/119038894): Simplify the 'for example' by using a factory method when it's ready public abstract boolean compare(@NullableDecl A actual, @NullableDecl E expected); diff --git a/core/src/main/java/com/google/common/truth/MultimapSubject.java b/core/src/main/java/com/google/common/truth/MultimapSubject.java index 6472aab4f..7232f7c6d 100644 --- a/core/src/main/java/com/google/common/truth/MultimapSubject.java +++ b/core/src/main/java/com/google/common/truth/MultimapSubject.java @@ -19,7 +19,9 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Strings.lenientFormat; import static com.google.common.collect.Maps.immutableEntry; +import static com.google.common.truth.Fact.fact; import static com.google.common.truth.Fact.simpleFact; +import static com.google.common.truth.Facts.facts; import static com.google.common.truth.SubjectUtils.HUMAN_UNDERSTANDABLE_EMPTY_STRING; import static com.google.common.truth.SubjectUtils.countDuplicatesAndAddTypeInfo; import static com.google.common.truth.SubjectUtils.hasMatchingToStringPair; @@ -475,43 +477,74 @@ public void containsEntry(@NullableDecl Object expectedKey, @NullableDecl E expe if (actual().containsKey(expectedKey)) { // Found matching key. Collection actualValues = getCastActual().asMap().get(expectedKey); + Correspondence.ExceptionStore compareExceptions = + Correspondence.ExceptionStore.forMapValuesCompare(); for (A actualValue : actualValues) { - if (correspondence.compare(actualValue, expectedValue)) { - // Found matching key and value. Test passes! + if (correspondence.safeCompare(actualValue, expectedValue, compareExceptions)) { + // Found matching key and value, but we still need to fail if we hit an exception along + // the way. + if (!compareExceptions.isEmpty()) { + failWithActual( + compareExceptions + .describeAsMainCause() + .and( + simpleFact( + "comparing contents by testing that at least one entry had a key " + + "equal to the expected key and a value that " + + correspondence + + " the expected value"), + fact("expected key", expectedKey), + fact("expected value", expectedValue))); + } return; } } // Found matching key with non-matching values. failWithoutActual( - simpleFact( - lenientFormat( - "Not true that %s contains at least one entry with key <%s> and a value that " - + "%s <%s>. However, it has a mapping from that key to <%s>", - actualAsString(), expectedKey, correspondence, expectedValue, actualValues))); + facts( + simpleFact( + lenientFormat( + "Not true that %s contains at least one entry with key <%s> and a " + + "value that %s <%s>. However, it has a mapping from that key to " + + "<%s>", + actualAsString(), + expectedKey, + correspondence, + expectedValue, + actualValues))) + .and(compareExceptions.describeAsAdditionalInfo())); } else { // Did not find matching key. Set keys = new LinkedHashSet<>(); + Correspondence.ExceptionStore compareExceptions = + Correspondence.ExceptionStore.forMapValuesCompare(); for (Entry actualEntry : getCastActual().entries()) { - if (correspondence.compare(actualEntry.getValue(), expectedValue)) { + if (correspondence.safeCompare( + actualEntry.getValue(), expectedValue, compareExceptions)) { keys.add(actualEntry.getKey()); } } if (!keys.isEmpty()) { // Found matching values with non-matching keys. failWithoutActual( - simpleFact( - lenientFormat( - "Not true that %s contains at least one entry with key <%s> and a value that " - + "%s <%s>. However, the following keys are mapped to such values: <%s>", - actualAsString(), expectedKey, correspondence, expectedValue, keys))); + facts( + simpleFact( + lenientFormat( + "Not true that %s contains at least one entry with key <%s> and a " + + "value that %s <%s>. However, the following keys are mapped to " + + "such values: <%s>", + actualAsString(), expectedKey, correspondence, expectedValue, keys))) + .and(compareExceptions.describeAsAdditionalInfo())); } else { // Did not find matching key or value. failWithoutActual( - simpleFact( - lenientFormat( - "Not true that %s contains at least one entry with key <%s> and a value that " - + "%s <%s>", - actualAsString(), expectedKey, correspondence, expectedValue))); + facts( + simpleFact( + lenientFormat( + "Not true that %s contains at least one entry with key <%s> and a " + + "value that %s <%s>", + actualAsString(), expectedKey, correspondence, expectedValue))) + .and(compareExceptions.describeAsAdditionalInfo())); } } } @@ -525,22 +558,43 @@ public void doesNotContainEntry( if (actual().containsKey(excludedKey)) { Collection actualValues = getCastActual().asMap().get(excludedKey); List matchingValues = new ArrayList<>(); + Correspondence.ExceptionStore compareExceptions = + Correspondence.ExceptionStore.forMapValuesCompare(); for (A actualValue : actualValues) { - if (correspondence.compare(actualValue, excludedValue)) { + if (correspondence.safeCompare(actualValue, excludedValue, compareExceptions)) { matchingValues.add(actualValue); } } + // Fail if we found a matching value for the key. if (!matchingValues.isEmpty()) { failWithoutActual( - simpleFact( - lenientFormat( - "Not true that %s did not contain an entry with key <%s> and a value that %s <%s>. " - + "It maps that key to the following such values: <%s>", - actualAsString(), - excludedKey, - correspondence, - excludedValue, - matchingValues))); + facts( + simpleFact( + lenientFormat( + "Not true that %s did not contain an entry with key <%s> and a value " + + "that %s <%s>. It maps that key to the following such values: " + + "<%s>", + actualAsString(), + excludedKey, + correspondence, + excludedValue, + matchingValues))) + .and(compareExceptions.describeAsAdditionalInfo())); + } else { + // No value matched, but we still need to fail if we hit an exception along the way. + if (!compareExceptions.isEmpty()) { + failWithActual( + compareExceptions + .describeAsMainCause() + .and( + simpleFact( + "comparing contents by testing that no entry had the forbidden key and " + + "a value that " + + correspondence + + " the forbidden value"), + fact("forbidden key", excludedKey), + fact("forbidden value", excludedValue))); + } } } } diff --git a/core/src/test/java/com/google/common/truth/MultimapSubjectTest.java b/core/src/test/java/com/google/common/truth/MultimapSubjectTest.java index 9563459c5..373df2238 100644 --- a/core/src/test/java/com/google/common/truth/MultimapSubjectTest.java +++ b/core/src/test/java/com/google/common/truth/MultimapSubjectTest.java @@ -16,6 +16,8 @@ package com.google.common.truth; import static com.google.common.base.Strings.lenientFormat; +import static com.google.common.truth.TestCorrespondences.CASE_INSENSITIVE_EQUALITY; +import static com.google.common.truth.TestCorrespondences.CASE_INSENSITIVE_EQUALITY_HALF_NULL_SAFE; import static com.google.common.truth.TestCorrespondences.STRING_PARSES_TO_INTEGER_CORRESPONDENCE; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; @@ -851,17 +853,97 @@ public void comparingValuesUsing_containsEntry_failsMissingExpectedKeyAndValue() + "key and a value that parses to <321>"); } + @Test + public void comparingValuesUsing_containsEntry_handlesException_expectedKeyHasWrongValues() { + ListMultimap actual = LinkedListMultimap.create(); + actual.put(1, "one"); + actual.put(2, "two"); + actual.put(2, "deux"); + actual.put(2, null); + expectFailureWhenTestingThat(actual) + .comparingValuesUsing(CASE_INSENSITIVE_EQUALITY) + .containsEntry(2, "ZWEI"); + // The test fails because the expected key doesn't have a match for the expected value. We are + // bound also to hit a NPE from compare(null, ZWEI) along the way, and should also report that. + assertFailureKeys( + "Not true that <{1=[one], 2=[two, deux, null]}> contains at least one entry with key <2> " + + "and a value that equals (ignoring case) . However, it has a mapping from that " + + "key to <[two, deux, null]>", + "additionally, one or more exceptions were thrown while comparing values", + "first exception"); + assertThatFailure() + .factValue("first exception") + .startsWith("compare(null, ZWEI) threw java.lang.NullPointerException"); + } + + @Test + public void comparingValuesUsing_containsEntry_handlesException_wrongKeyHasExpectedValue() { + ListMultimap actual = LinkedListMultimap.create(); + actual.put(1, "one"); + actual.put(3, "two"); + actual.put(3, null); + actual.put(3, "zwei"); + expectFailureWhenTestingThat(actual) + .comparingValuesUsing(CASE_INSENSITIVE_EQUALITY) + .containsEntry(2, "ZWEI"); + // The test fails and does not contain the expected key, but does contain the expected value + // we the wrong key. We are bound also to hit a NPE from compare(null, ZWEI) along the way, and + // should also report that. + assertFailureKeys( + "Not true that <{1=[one], 3=[two, null, zwei]}> contains at least one entry with key <2> " + + "and a value that equals (ignoring case) . However, the following keys are " + + "mapped to such values: <[3]>", + "additionally, one or more exceptions were thrown while comparing values", + "first exception"); + assertThatFailure() + .factValue("first exception") + .startsWith("compare(null, ZWEI) threw java.lang.NullPointerException"); + } + + @Test + public void comparingValuesUsing_containsEntry_handlesException_alwaysFails() { + ListMultimap actual = LinkedListMultimap.create(); + actual.put(1, "one"); + actual.put(2, "two"); + actual.put(2, null); + actual.put(2, "zwei"); + expectFailureWhenTestingThat(actual) + .comparingValuesUsing(CASE_INSENSITIVE_EQUALITY) + .containsEntry(2, "ZWEI"); + // The multimap does contain the expected entry, but no reasonable implementation could find + // it without hitting the NPE from compare(null, ZWEI) first, so we are contractually required + // to fail. + assertFailureKeys( + "one or more exceptions were thrown while comparing values", + "first exception", + "comparing contents by testing that at least one entry had a key equal to the expected key " + + "and a value that equals (ignoring case) the expected value", + "expected key", + "expected value", + "but was"); + assertThatFailure() + .factValue("first exception") + .startsWith("compare(null, ZWEI) threw java.lang.NullPointerException"); + assertFailureValue("expected key", "2"); + assertFailureValue("expected value", "ZWEI"); + } + @Test public void comparingValuesUsing_containsEntry_wrongTypeInActual() { ImmutableListMultimap actual = - ImmutableListMultimap.of("abc", "+123", "def", "+456", "def", new Object()); - try { - assertThat(actual) - .comparingValuesUsing(STRING_PARSES_TO_INTEGER_CORRESPONDENCE) - .containsEntry("def", 123); - fail("Should have thrown."); - } catch (ClassCastException expected) { - } + ImmutableListMultimap.of("abc", "+123", "def", "+456", "def", 789); + expectFailureWhenTestingThat(actual) + .comparingValuesUsing(STRING_PARSES_TO_INTEGER_CORRESPONDENCE) + .containsEntry("def", 789); + assertFailureKeys( + "Not true that <{abc=[+123], def=[+456, 789]}> contains at least one entry with key " + + "and a value that parses to <789>. However, it has a mapping from that key to " + + "<[+456, 789]>", + "additionally, one or more exceptions were thrown while comparing values", + "first exception"); + assertThatFailure() + .factValue("first exception") + .startsWith("compare(789, 789) threw java.lang.ClassCastException"); } @Test @@ -906,17 +988,76 @@ public void comparingValuesUsing_doesNotContainEntry_failure() { + "It maps that key to the following such values: <[+789]>"); } + @Test + public void comparingValuesUsing_doesNotContainEntry_handlesException_didContainEntry() { + ListMultimap actual = LinkedListMultimap.create(); + actual.put(1, "one"); + actual.put(2, "two"); + actual.put(2, null); + actual.put(2, "zwei"); + expectFailureWhenTestingThat(actual) + .comparingValuesUsing(CASE_INSENSITIVE_EQUALITY) + .doesNotContainEntry(2, "ZWEI"); + // The test fails because it does contain the expected entry. We are bound to also hit the NPE + // from compare(null, ZWEI) along the way, and should also report that. + assertFailureKeys( + "Not true that <{1=[one], 2=[two, null, zwei]}> did not contain an entry with key <2> and " + + "a value that equals (ignoring case) . It maps that key to the following such " + + "values: <[zwei]>", + "additionally, one or more exceptions were thrown while comparing values", + "first exception"); + assertThatFailure() + .factValue("first exception") + .startsWith("compare(null, ZWEI) threw java.lang.NullPointerException"); + } + + @Test + public void comparingValuesUsing_doesNotContainEntry_handlesException_didNotContainEntry() { + ListMultimap actual = LinkedListMultimap.create(); + actual.put(1, "one"); + actual.put(2, "two"); + actual.put(2, "deux"); + actual.put(2, null); + expectFailureWhenTestingThat(actual) + .comparingValuesUsing(CASE_INSENSITIVE_EQUALITY) + .doesNotContainEntry(2, "ZWEI"); + // The test would pass if compare(null, ZWEI) returned false. But it actually throws NPE, and + // we are bound to hit that, so we are contractually required to fail. + assertFailureKeys( + "one or more exceptions were thrown while comparing values", + "first exception", + "comparing contents by testing that no entry had the forbidden key and a value that " + + "equals (ignoring case) the forbidden value", + "forbidden key", + "forbidden value", + "but was"); + assertThatFailure() + .factValue("first exception") + .startsWith("compare(null, ZWEI) threw java.lang.NullPointerException"); + assertFailureValue("forbidden key", "2"); + assertFailureValue("forbidden value", "ZWEI"); + } + @Test public void comparingValuesUsing_doesNotContainEntry_wrongTypeInActual() { ImmutableListMultimap actual = - ImmutableListMultimap.of("abc", "+123", "def", "+456", "def", new Object()); - try { - assertThat(actual) - .comparingValuesUsing(STRING_PARSES_TO_INTEGER_CORRESPONDENCE) - .doesNotContainEntry("def", 789); - fail("Should have thrown."); - } catch (ClassCastException expected) { - } + ImmutableListMultimap.of("abc", "+123", "def", "+456", "def", 789); + expectFailureWhenTestingThat(actual) + .comparingValuesUsing(STRING_PARSES_TO_INTEGER_CORRESPONDENCE) + .doesNotContainEntry("def", 789); + assertFailureKeys( + "one or more exceptions were thrown while comparing values", + "first exception", + "comparing contents by testing that no entry had the forbidden key and a value that " + + "parses to the forbidden value", + "forbidden key", + "forbidden value", + "but was"); + assertThatFailure() + .factValue("first exception") + .startsWith("compare(789, 789) threw java.lang.ClassCastException"); + assertFailureValue("forbidden key", "def"); + assertFailureValue("forbidden value", "789"); } @Test @@ -991,6 +1132,63 @@ public void comparingValuesUsing_containsExactlyEntriesIn_wrongValueForKey() { .isAnyOf(expectedPreamble + "<[def=+64]>", expectedPreamble + "<[def=0x40]>"); } + @Test + public void comparingValuesUsing_containsExactlyEntriesIn_handlesException() { + ListMultimap actual = LinkedListMultimap.create(); + actual.put(1, "one"); + actual.put(2, null); + actual.put(2, "deux"); + actual.put(2, "zwei"); + ImmutableListMultimap expected = + ImmutableListMultimap.of(1, "ONE", 2, "TWO", 2, "DEUX", 2, "ZWEI"); + expectFailureWhenTestingThat(actual) + .comparingValuesUsing(CASE_INSENSITIVE_EQUALITY) + .containsExactlyEntriesIn(expected); + assertFailureKeys( + "Not true that <{1=[one], 2=[null, deux, zwei]}> contains exactly one element that has a " + + "key that is equal to and a value that equals (ignoring case) the key and value of " + + "each element of <[1=ONE, 2=TWO, 2=DEUX, 2=ZWEI]>. It is missing an element that has " + + "a key that is equal to and a value that equals (ignoring case) the key and value of " + + "<2=TWO> and has unexpected elements <[2=null]>", + "additionally, one or more exceptions were thrown while comparing elements", + "first exception"); + assertThatFailure() + .factValue("first exception") + .startsWith("compare(2=null, 2=TWO) threw java.lang.NullPointerException"); + } + + @Test + public void comparingValuesUsing_containsExactlyEntriesIn_handlesException_alwaysFails() { + ListMultimap actual = LinkedListMultimap.create(); + actual.put(1, "one"); + actual.put(2, null); + actual.put(2, "two"); + actual.put(2, "deux"); + ListMultimap expected = LinkedListMultimap.create(); + expected.put(1, "ONE"); + expected.put(2, "TWO"); + expected.put(2, "DEUX"); + expected.put(2, null); + expectFailureWhenTestingThat(actual) + .comparingValuesUsing(CASE_INSENSITIVE_EQUALITY_HALF_NULL_SAFE) + .containsExactlyEntriesIn(expected); + // CASE_INSENSITIVE_EQUALITY_HALF_NULL_SAFE.compare(null, null) returns true, so there is a + // mapping between actual and expected entries where they all correspond. However, no + // reasonable implementation would find that mapping without hitting the (null, "TWO") case + // along the way, and that throws NPE, so we are contractually required to fail. + assertFailureKeys( + "one or more exceptions were thrown while comparing elements", + "first exception", + "comparing contents by testing that each element has a key that is equal to and a value " + + "that equals (ignoring case) the key and value of an expected value", + "expected", + "but was"); + assertThatFailure() + .factValue("first exception") + .startsWith("compare(2=null, 2=TWO) threw java.lang.NullPointerException"); + assertFailureValue("expected", "[1=ONE, 2=TWO, 2=DEUX, 2=null]"); + } + @Test public void comparingValuesUsing_containsExactlyEntriesIn_wrongTypeInActual() { ImmutableListMultimap actual =