Skip to content
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.

Sign up for GitHub

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

Jump to bottom

MapSubject#containsExactlyEntriesIn throws NPE for null values of keys #468

Closed
jirkadanek opened this issue Jun 20, 2018 · 10 comments
Closed

MapSubject#containsExactlyEntriesIn throws NPE for null values of keys #468

jirkadanek opened this issue Jun 20, 2018 · 10 comments
Milestone
0.42

Comments

@jirkadanek
Copy link

Version: Truth 0.41

Consider the following test

    @Test
    fun `compare map with null value`() {
        Truth.assertThat(mapOf("key" to "value")).containsExactlyEntriesIn(mapOf("key" to null))
    }

This test fails with the wrong message. I get a NPE from within Truth, instead of the expected Assertion exception ("value" != null).

java.lang.NullPointerException
	at com.google.common.truth.MapSubject$2.apply(MapSubject.java:351)
	at com.google.common.truth.MapSubject$2.apply(MapSubject.java:348)
	at com.google.common.collect.Maps$9.transformEntry(Maps.java:1821)
	at com.google.common.collect.Maps$12.getValue(Maps.java:1862)
	at java.base/java.util.AbstractMap.toString(AbstractMap.java:553)
	at java.base/java.lang.String.valueOf(String.java:2788)
	at java.base/java.lang.StringBuilder.append(StringBuilder.java:135)
	at com.google.common.truth.MapSubject$MapDifference.describe(MapSubject.java:320)
	at com.google.common.truth.MapSubject.containsExactlyEntriesInAnyOrder(MapSubject.java:240)
	at com.google.common.truth.MapSubject.containsExactlyEntriesIn(MapSubject.java:223)
	at InteropTest.compare map with null value(InteropTest.kt:78)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:564)
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:436)
	at org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:115)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeTestMethod$6(TestMethodTestDescriptor.java:170)
	at org.junit.jupiter.engine.execution.ThrowableCollector.execute(ThrowableCollector.java:40)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeTestMethod(TestMethodTestDescriptor.java:166)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:113)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:58)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor$NodeExecutor.lambda$executeRecursively$3(HierarchicalTestExecutor.java:112)
	at org.junit.platform.engine.support.hierarchical.SingleTestExecutor.executeSafely(SingleTestExecutor.java:66)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor$NodeExecutor.executeRecursively(HierarchicalTestExecutor.java:108)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor$NodeExecutor.execute(HierarchicalTestExecutor.java:79)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor$NodeExecutor.lambda$executeRecursively$2(HierarchicalTestExecutor.java:120)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)
	at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:177)
	at java.base/java.util.Iterator.forEachRemaining(Iterator.java:133)
	at java.base/java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1801)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
	at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor$NodeExecutor.lambda$executeRecursively$3(HierarchicalTestExecutor.java:120)
	at org.junit.platform.engine.support.hierarchical.SingleTestExecutor.executeSafely(SingleTestExecutor.java:66)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor$NodeExecutor.executeRecursively(HierarchicalTestExecutor.java:108)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor$NodeExecutor.execute(HierarchicalTestExecutor.java:79)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor$NodeExecutor.lambda$executeRecursively$2(HierarchicalTestExecutor.java:120)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)
	at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:177)
	at java.base/java.util.Iterator.forEachRemaining(Iterator.java:133)
	at java.base/java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1801)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
	at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor$NodeExecutor.lambda$executeRecursively$3(HierarchicalTestExecutor.java:120)
	at org.junit.platform.engine.support.hierarchical.SingleTestExecutor.executeSafely(SingleTestExecutor.java:66)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor$NodeExecutor.executeRecursively(HierarchicalTestExecutor.java:108)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor$NodeExecutor.execute(HierarchicalTestExecutor.java:79)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:55)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:43)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:170)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:154)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:90)
	at com.intellij.junit5.JUnit5IdeaTestRunner.startRunnerWithArgs(JUnit5IdeaTestRunner.java:74)
	at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)

The NPE originates in com.google.common.truth.MapSubject:

public class MapSubject extends Subject<MapSubject, Map<?, ?>> {
    private static final MapSubject.ValueTester<Object, Object> EQUALITY = new MapSubject.ValueTester<Object, Object>() {
        public boolean test(@Nullable Object actualValue, @Nullable Object expectedValue) {
            return Objects.equal(actualValue, expectedValue);
        }
    };
    private static final Function<MapSubject.ValueDifference<Object, Object>, String> VALUE_DIFFERENCE_FORMAT = new Function<MapSubject.ValueDifference<Object, Object>, String>() {
        public String apply(MapSubject.ValueDifference<Object, Object> values) {
/* HERE */  boolean includeTypes = values.actual.toString().equals(values.expected.toString());
            return StringUtil.format("(expected %s but got %s)", new Object[]{includeTypes ? new MapSubject.TypedToStringWrapper(values.expected) : values.expected, includeTypes ? new MapSubject.TypedToStringWrapper(values.actual) : values.actual});
        }
    };
@jirkadanek jirkadanek changed the title MapSublect#containsExactlyEntriesIn throws NPE for null values of keys MapSubject#containsExactlyEntriesIn throws NPE for null values of keys Jun 20, 2018
@jrtom
Copy link
Member

jrtom commented Jun 20, 2018

Given the mapOf operator you're using, I'm assuming that you're using this with Kotlin.

How does Kotlin define hashCode() and equals() for maps created in this way?

@lowasser
Copy link
Contributor

mapOf returns a java.util.HashMap.

@cpovirk
Copy link
Member

cpovirk commented Jun 20, 2018

Probably we should be calling String.valueOf(actual|expected) instead of (actual|expected).toString(). Thanks for the report.

@cpovirk
Copy link
Member

cpovirk commented Jun 20, 2018

(@PeteGillin if he wants to look at this. Otherwise, I'll get to it eventually.)

@jirkadanek
Copy link
Author

@lowasser Thanks. Kotlin is irrelevant here, I can get the same error with Java 8:

    @Test
    void testComparingMapWithNullValues() {
        Map<String, String> m = new HashMap<>();
        Map<String, String> n = new HashMap<>();
        m.put("key", "value");
        n.put("key", null);
        Truth.assertThat(m).containsExactlyEntriesIn(n);
    }
java.lang.NullPointerException
	at com.google.common.truth.MapSubject$2.apply(MapSubject.java:351)
	at com.google.common.truth.MapSubject$2.apply(MapSubject.java:348)
	at com.google.common.collect.Maps$9.transformEntry(Maps.java:1821)
	at com.google.common.collect.Maps$12.getValue(Maps.java:1862)
	at java.base/java.util.AbstractMap.toString(AbstractMap.java:553)
	at java.base/java.lang.String.valueOf(String.java:2788)
	at java.base/java.lang.StringBuilder.append(StringBuilder.java:135)
	at com.google.common.truth.MapSubject$MapDifference.describe(MapSubject.java:320)
	at com.google.common.truth.MapSubject.containsExactlyEntriesInAnyOrder(MapSubject.java:240)
	at com.google.common.truth.MapSubject.containsExactlyEntriesIn(MapSubject.java:223)
	at InteropTestJava.testComparingMapWithNullValues(InteropTest.java:21)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[...]

@PeteGillin
Copy link

Yeah, this is my fault. I originally introduced it back in December (aad4f3a). I agree with the fix @cpovirk proposed to use String.valueOf. I'll see if I can throw a fix together today or tomorrow.

I'm vaguely surprised we haven't seen this before, since AFAICS it affects any failing isEqualTo, containsExactly, or containsExactlyElementsIn (non-fuzzy) on any Map with null values in it. I guess people really don't have null values very often?

Anyway, thanks for the report, and sorry for the trouble.

@PeteGillin
Copy link

(Actually, not "any failing" test of those forms, only ones where it has the right key with the wrong value. Nevertheless.)

I had a look around, and I don't think that we have similar issues in IterableSubject or MultimapSubject, or MapSubject's fuzzy assertions.

@PeteGillin
Copy link

FYI, this was fixed internally on Friday, I think we have to wait for it to get synced here.

@PeteGillin
Copy link

This is now fixed at github master in bf2cc0d . (Sorry for the delay, we had a slight administrative hitch with the syncing process.)

Assuming everyone is happy with this, can someone who has the appropriate permissions mark this as fixed.

@cpovirk cpovirk closed this as completed Jun 28, 2018
@cpovirk cpovirk added this to the 0.42 milestone Jul 12, 2018
@cpovirk
Copy link
Member

cpovirk commented Jul 12, 2018

Fix released in Truth 0.42.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.42
Development

No branches or pull requests
5 participants
@lowasser @cpovirk @PeteGillin @jrtom @jirkadanek