From 5198d89516668ffad80800cf067d8a6b8f3aab16 Mon Sep 17 00:00:00 2001 From: Alexander Akbashev Date: Wed, 2 May 2018 11:53:49 +0200 Subject: [PATCH 1/3] Adds tests to reproduce hashcode collision in build memory --- .../gerritnotifier/model/BuildMemory.java | 12 ++- .../model/BuildMemoryReportContainerTest.java | 99 +++++++++++++++++++ .../GerritTriggeredEventComparatorTest.java | 72 -------------- 3 files changed, 110 insertions(+), 73 deletions(-) create mode 100644 src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/BuildMemoryReportContainerTest.java delete mode 100644 src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/GerritTriggeredEventComparatorTest.java diff --git a/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/BuildMemory.java b/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/BuildMemory.java index e4dc9aa39..8aeb26ccd 100644 --- a/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/BuildMemory.java +++ b/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/BuildMemory.java @@ -24,6 +24,7 @@ */ package com.sonyericsson.hudson.plugins.gerrit.trigger.gerritnotifier.model; +import com.google.common.annotations.VisibleForTesting; import com.infradna.tool.bridge_method_injector.WithBridgeMethods; import com.sonyericsson.hudson.plugins.gerrit.trigger.diagnostics.BuildMemoryReport; import com.sonyericsson.hudson.plugins.gerrit.trigger.gerritnotifier.model.BuildMemory.MemoryImprint.Entry; @@ -81,7 +82,7 @@ public int compare(GerritTriggeredEvent o1, GerritTriggeredEvent o2) { } } - private TreeMap memory = + private Map memory = new TreeMap( new GerritTriggeredEventComparator()); private static final Logger logger = LoggerFactory.getLogger(BuildMemory.class); @@ -96,6 +97,15 @@ public synchronized MemoryImprint getMemoryImprint(GerritTriggeredEvent event) { return memory.get(event); } + /** + * Gets the entire memory representation for all events + * @return memory representation for all events. + */ + @VisibleForTesting + Map getMemory() { + return memory; + } + /** * Tells if all triggered builds have started for a specific memory imprint. * diff --git a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/BuildMemoryReportContainerTest.java b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/BuildMemoryReportContainerTest.java new file mode 100644 index 000000000..0cc7d6c0d --- /dev/null +++ b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/BuildMemoryReportContainerTest.java @@ -0,0 +1,99 @@ +package com.sonyericsson.hudson.plugins.gerrit.trigger.gerritnotifier.model; + +import com.sonymobile.tools.gerrit.gerritevents.dto.events.ChangeBasedEvent; +import com.sonymobile.tools.gerrit.gerritevents.dto.events.GerritTriggeredEvent; +import com.sonymobile.tools.gerrit.gerritevents.dto.events.PatchsetCreated; +import hudson.model.AbstractProject; +import hudson.model.ItemGroup; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.mockito.Mockito; + +import java.util.Arrays; + +import static org.junit.Assert.assertEquals; + +/** + * Tests {@link BuildMemory} internal container. + * The main requirement is not merging non-equal events with same hash code. + * + * @author Robert Sandell <rsandell@cloudbees.com>. + */ +@RunWith(Parameterized.class) +public class BuildMemoryReportContainerTest { + + private final GerritTriggeredEvent o1; + private final GerritTriggeredEvent o2; + private final int expected; + private final BuildMemory memory; + + static class PatchsetCreatedWithPrefinedHash extends PatchsetCreated { + private final int hashCode; + + private PatchsetCreatedWithPrefinedHash(int hashCode) { + this.hashCode = hashCode; + } + + @Override + public boolean equals(Object o) { + return false; + } + + @Override + public int hashCode() { + return hashCode; + } + } + + /** + * The different scenarios. + * + * @return a list of parameters to the constructor. + * @see Parameterized + */ + @Parameterized.Parameters(name = "{index}: {0}, {1} == {2}") + public static Iterable permutations() { + ChangeBasedEvent event1 = new PatchsetCreatedWithPrefinedHash(777); + ChangeBasedEvent event2 = new PatchsetCreatedWithPrefinedHash(777); + return Arrays.asList(new Object[][]{ + {null, null, 1}, + {null, event1, 2}, + {event1, null, 2}, + {event1, event1, 1}, + {event2, event2, 1}, + {event1, event2, 2}, + {event2, event1, 2}, + }); + } + + /** + * Constructor. + * @param o1 first argument + * @param o2 second argument + * @param expected the expected result + */ + public BuildMemoryReportContainerTest(GerritTriggeredEvent o1, GerritTriggeredEvent o2, int expected) { + this.o1 = o1; + this.o2 = o2; + this.expected = expected; + memory = new BuildMemory(); + } + + /** + * Tests the scenario on {@link BuildMemory}. + * + * @throws Exception if so + */ + @Test + public void testCompare() throws Exception { + AbstractProject mock = Mockito.mock(AbstractProject.class); + ItemGroup itemGroup = Mockito.mock(ItemGroup.class); + Mockito.when(itemGroup.getFullName()).thenReturn("Parent"); + Mockito.when(mock.getParent()).thenReturn(itemGroup); + memory.triggered(o1, mock); + memory.triggered(o2, mock); + + assertEquals(expected, memory.getMemory().size()); + } +} diff --git a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/GerritTriggeredEventComparatorTest.java b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/GerritTriggeredEventComparatorTest.java deleted file mode 100644 index 8dbc44ec7..000000000 --- a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/GerritTriggeredEventComparatorTest.java +++ /dev/null @@ -1,72 +0,0 @@ -package com.sonyericsson.hudson.plugins.gerrit.trigger.gerritnotifier.model; - -import com.sonyericsson.hudson.plugins.gerrit.trigger.mock.Setup; -import com.sonymobile.tools.gerrit.gerritevents.dto.events.ChangeMerged; -import com.sonymobile.tools.gerrit.gerritevents.dto.events.GerritTriggeredEvent; -import com.sonymobile.tools.gerrit.gerritevents.dto.events.PatchsetCreated; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.Parameterized; - -import java.util.Arrays; -import java.util.Comparator; - -import static org.junit.Assert.assertEquals; - -/** - * Tests {@link BuildMemory.GerritTriggeredEventComparator}. - * - * @author Robert Sandell <rsandell@cloudbees.com>. - */ -@RunWith(Parameterized.class) -public class GerritTriggeredEventComparatorTest { - - private final GerritTriggeredEvent o1; - private final GerritTriggeredEvent o2; - private final int expected; - private final Comparator comparator; - - /** - * The different scenarios. - * - * @return a list of parameters to the constructor. - * @see Parameterized - */ - @Parameterized.Parameters(name = "{index}: {0}, {1} == {2}") - public static Iterable permutations() { - PatchsetCreated event = Setup.createPatchsetCreated(); - ChangeMerged merged = Setup.createChangeMerged(); - return Arrays.asList(new Object[][]{ - {null, null, 0}, - {null, event, -1}, - {event, null, 1}, - {event, event, 0}, - {event, merged, new Integer(event.hashCode()).compareTo(merged.hashCode())}, - {merged, event, new Integer(merged.hashCode()).compareTo(event.hashCode()), }, - }); - } - - /** - * Constructor. - * @param o1 first argument - * @param o2 second argument - * @param expected the expected result - */ - public GerritTriggeredEventComparatorTest(GerritTriggeredEvent o1, GerritTriggeredEvent o2, int expected) { - this.o1 = o1; - this.o2 = o2; - this.expected = expected; - comparator = new BuildMemory.GerritTriggeredEventComparator(); - } - - /** - * Tests the scenario on - * {@link BuildMemory.GerritTriggeredEventComparator#compare(GerritTriggeredEvent, GerritTriggeredEvent)}. - * - * @throws Exception if so - */ - @Test - public void testCompare() throws Exception { - assertEquals(expected, comparator.compare(o1, o2)); - } -} From ec4b7a42bcbf9af162899223dc6db4a3f900d47a Mon Sep 17 00:00:00 2001 From: Alexander Akbashev Date: Wed, 2 May 2018 13:15:01 +0200 Subject: [PATCH 2/3] Switches internal containter of BuildMemory to HashMap That solves a problem of hash collision for non-equal objects. --- .../gerritnotifier/model/BuildMemory.java | 30 ++----------------- 1 file changed, 2 insertions(+), 28 deletions(-) diff --git a/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/BuildMemory.java b/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/BuildMemory.java index 8aeb26ccd..85b3c5920 100644 --- a/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/BuildMemory.java +++ b/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/BuildMemory.java @@ -45,11 +45,10 @@ import javax.annotation.Nonnull; import java.io.IOException; import java.util.ArrayList; -import java.util.Comparator; import java.util.LinkedList; import java.util.List; import java.util.Map; -import java.util.TreeMap; +import java.util.HashMap; import static com.sonyericsson.hudson.plugins.gerrit.trigger.utils.Logic.shouldSkip; @@ -59,32 +58,7 @@ * @author Robert Sandell <robert.sandell@sonyericsson.com> */ public class BuildMemory { - - /** - * Compares GerritTriggeredEvents using the Object.hashCode() method. This ensures that every event received from - * Gerrit is kept track of individually. - * - * @author James E. Blair <jeblair@hp.com> - */ - static class GerritTriggeredEventComparator implements Comparator { - @Override - public int compare(GerritTriggeredEvent o1, GerritTriggeredEvent o2) { - if (o1 == null && o2 == null) { - return 0; - } - if (o1 != null && o2 == null) { - return 1; - } - if (o1 == null && o2 != null) { - return -1; - } - return Integer.valueOf(o1.hashCode()).compareTo(o2.hashCode()); - } - } - - private Map memory = - new TreeMap( - new GerritTriggeredEventComparator()); + private Map memory = new HashMap(); private static final Logger logger = LoggerFactory.getLogger(BuildMemory.class); /** From 6c59ee5f69b8e20e9af35b1806e13c8ee0963b94 Mon Sep 17 00:00:00 2001 From: Alexander Akbashev Date: Wed, 2 May 2018 13:59:01 +0200 Subject: [PATCH 3/3] Improve readability of test class --- .../model/BuildMemoryReportContainerTest.java | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/BuildMemoryReportContainerTest.java b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/BuildMemoryReportContainerTest.java index 0cc7d6c0d..87dbfbd84 100644 --- a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/BuildMemoryReportContainerTest.java +++ b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/BuildMemoryReportContainerTest.java @@ -11,6 +11,7 @@ import org.mockito.Mockito; import java.util.Arrays; +import java.util.UUID; import static org.junit.Assert.assertEquals; @@ -28,22 +29,33 @@ public class BuildMemoryReportContainerTest { private final int expected; private final BuildMemory memory; - static class PatchsetCreatedWithPrefinedHash extends PatchsetCreated { + static class PatchsetCreatedWithPredefinedHash extends PatchsetCreated { private final int hashCode; + private final UUID uuid; - private PatchsetCreatedWithPrefinedHash(int hashCode) { + private PatchsetCreatedWithPredefinedHash(int hashCode) { this.hashCode = hashCode; + this.uuid = UUID.randomUUID(); } @Override public boolean equals(Object o) { - return false; + return o instanceof PatchsetCreatedWithPredefinedHash && + uuid.equals(((PatchsetCreatedWithPredefinedHash) o).uuid); + } @Override public int hashCode() { return hashCode; } + + @Override + public String toString() { + return "PatchsetCreatedWithPredefinedHash{" + + "uuid=" + uuid + + '}'; + } } /** @@ -54,8 +66,8 @@ public int hashCode() { */ @Parameterized.Parameters(name = "{index}: {0}, {1} == {2}") public static Iterable permutations() { - ChangeBasedEvent event1 = new PatchsetCreatedWithPrefinedHash(777); - ChangeBasedEvent event2 = new PatchsetCreatedWithPrefinedHash(777); + ChangeBasedEvent event1 = new PatchsetCreatedWithPredefinedHash(777); + ChangeBasedEvent event2 = new PatchsetCreatedWithPredefinedHash(777); return Arrays.asList(new Object[][]{ {null, null, 1}, {null, event1, 2},