From b71d3a7dcf392d91810c0f8cb633cc2821233136 Mon Sep 17 00:00:00 2001 From: Pijukatel Date: Tue, 4 Jul 2023 22:27:05 +0200 Subject: [PATCH 1/3] Fix JENKINS-66535 --- .../gerritnotifier/ParameterExpander.java | 20 ++++++++++++++----- .../rest/BuildCompletedRestCommandJob.java | 2 +- ...arameterExpanderSkipVoteParameterTest.java | 4 ++-- .../gerritnotifier/ParameterExpanderTest.java | 4 ++-- 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpander.java b/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpander.java index b2b5cf338..2eb5f1faf 100644 --- a/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpander.java +++ b/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpander.java @@ -381,12 +381,14 @@ protected Integer getVerifiedValue(Result res, GerritTrigger trigger) { /** * Returns the minimum verified value for the build results in the memory. * If no builds have contributed to verified value, this method returns null - * @param memoryImprint the memory. - * @param onlyBuilt only count builds that completed (no NOT_BUILT builds) + * + * @param memoryImprint the memory. + * @param onlyBuilt only count builds that completed (no NOT_BUILT builds) + * @param maxAllowedVerifiedValue Upper boundary on verified value. * @return the lowest verified value. */ @CheckForNull - public Integer getMinimumVerifiedValue(MemoryImprint memoryImprint, boolean onlyBuilt) { + public Integer getMinimumVerifiedValue(MemoryImprint memoryImprint, boolean onlyBuilt, Integer maxAllowedVerifiedValue) { Integer verified = Integer.MAX_VALUE; for (Entry entry : memoryImprint.getEntries()) { if (entry == null) { @@ -415,7 +417,7 @@ public Integer getMinimumVerifiedValue(MemoryImprint memoryImprint, boolean only return null; } - return verified; + return Math.min(verified, maxAllowedVerifiedValue); } /** @@ -531,12 +533,19 @@ public String getBuildCompletedCommand(MemoryImprint memoryImprint, TaskListener // builds were successful, unstable or failed, we find the minimum // verified/code review value for the NOT_BUILT ones too. boolean onlyCountBuilt = true; + // If some builds failed, but their verified score is no longer + // available, then Successful builds would bump up the score. + // Set upper boundary on verified values based on builds status. + // JENKINS-66535 + Integer maxAllowedVerifiedValue = Integer.MAX_VALUE; if (memoryImprint.wereAllBuildsSuccessful()) { command = config.getGerritCmdBuildSuccessful(); } else if (memoryImprint.wereAnyBuildsFailed()) { command = config.getGerritCmdBuildFailed(); + maxAllowedVerifiedValue = config.getGerritBuildFailedVerifiedValue(); } else if (memoryImprint.wereAnyBuildsUnstable()) { command = config.getGerritCmdBuildUnstable(); + maxAllowedVerifiedValue = config.getGerritBuildUnstableVerifiedValue(); } else if (memoryImprint.wereAllBuildsNotBuilt()) { onlyCountBuilt = false; command = config.getGerritCmdBuildNotBuilt(); @@ -545,13 +554,14 @@ public String getBuildCompletedCommand(MemoryImprint memoryImprint, TaskListener } else { //Just as bad as failed for now. command = config.getGerritCmdBuildFailed(); + maxAllowedVerifiedValue = config.getGerritBuildFailedVerifiedValue(); } Integer verified = null; Integer codeReview = null; Notify notifyLevel = Notify.ALL; if (memoryImprint.getEvent().isScorable()) { - verified = getMinimumVerifiedValue(memoryImprint, onlyCountBuilt); + verified = getMinimumVerifiedValue(memoryImprint, onlyCountBuilt, maxAllowedVerifiedValue); codeReview = getMinimumCodeReviewValue(memoryImprint, onlyCountBuilt); notifyLevel = getHighestNotificationLevel(memoryImprint, onlyCountBuilt); } diff --git a/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/job/rest/BuildCompletedRestCommandJob.java b/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/job/rest/BuildCompletedRestCommandJob.java index 14c5728b5..9ab3bc7eb 100644 --- a/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/job/rest/BuildCompletedRestCommandJob.java +++ b/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/job/rest/BuildCompletedRestCommandJob.java @@ -90,7 +90,7 @@ protected ReviewInput createReview() { } } if (config.isRestVerified()) { - Integer verValue = parameterExpander.getMinimumVerifiedValue(memoryImprint, true); + Integer verValue = parameterExpander.getMinimumVerifiedValue(memoryImprint, true, Integer.MAX_VALUE); if (verValue != null && verValue != Integer.MAX_VALUE) { scoredLabels.add(new ReviewLabel( LABEL_VERIFIED, diff --git a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderSkipVoteParameterTest.java b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderSkipVoteParameterTest.java index 8016f3037..d710a4bc1 100644 --- a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderSkipVoteParameterTest.java +++ b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderSkipVoteParameterTest.java @@ -103,14 +103,14 @@ public void testCodeReview() { } /** - * Tests that {@link ParameterExpander#getMinimumVerifiedValue(BuildMemory.MemoryImprint, boolean)} + * Tests that {@link ParameterExpander#getMinimumVerifiedValue(BuildMemory.MemoryImprint, boolean, Integer)} * returns {@link TestParameter#expectedVerified}. */ @Test public void testVerified() { IGerritHudsonTriggerConfig config = Setup.createConfig(); ParameterExpander instance = new ParameterExpander(config); - Integer result = instance.getMinimumVerifiedValue(parameter.memoryImprint, true); + Integer result = instance.getMinimumVerifiedValue(parameter.memoryImprint, true, Integer.MAX_VALUE); if (parameter.expectedVerified == null) { assertNull(result); } else { diff --git a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderTest.java b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderTest.java index 6e08b84da..49aef2403 100644 --- a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderTest.java +++ b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderTest.java @@ -178,12 +178,12 @@ public void testGetMinimumVerifiedValue() { // When not all results are NOT_BUILT, we should ignore NOT_BUILT. int expResult = -1; - int result = instance.getMinimumVerifiedValue(memoryImprint, true); + int result = instance.getMinimumVerifiedValue(memoryImprint, true, Integer.MAX_VALUE); assertEquals(expResult, result); // Otherwise, we should use NOT_BUILT. expResult = -4; - result = instance.getMinimumVerifiedValue(memoryImprint, false); + result = instance.getMinimumVerifiedValue(memoryImprint, false, Integer.MAX_VALUE); assertEquals(expResult, result); } From 26e29fc82310d9108d3e8ffa784426d635e81ea8 Mon Sep 17 00:00:00 2001 From: Pijukatel Date: Sun, 16 Jul 2023 18:12:48 +0200 Subject: [PATCH 2/3] Add tests for verified value saturation in case of missing failed build. --- .../gerritnotifier/ParameterExpanderTest.java | 71 +++++++++++++++++++ .../plugins/gerrit/trigger/mock/Setup.java | 6 ++ 2 files changed, 77 insertions(+) diff --git a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderTest.java b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderTest.java index 49aef2403..4e1b6a1d8 100644 --- a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderTest.java +++ b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderTest.java @@ -376,6 +376,35 @@ public void testGetMinimumCodeReviewValueForOneJobOverridenMixed() { assertEquals(Integer.valueOf(2), result); } + /** + * Tests {@link ParameterExpander#getMinimumVerifiedValue(MemoryImprint, boolean, Integer)} with two + * jobs. One successful build, the other failed missing build (null). + * + */ + @Test + public void testGetVerifiedValueOneSuccessJobAndMissingFailedJob() { + IGerritHudsonTriggerConfig config = Setup.createConfigWithCodeReviewsNull(); + + ParameterExpander instance = new ParameterExpander(config, jenkins); + MemoryImprint memoryImprint = mock(MemoryImprint.class); + MemoryImprint.Entry[] entries = new MemoryImprint.Entry[2]; + + GerritTrigger trigger = mock(GerritTrigger.class); + when(trigger.getGerritBuildFailedVerifiedValue()).thenReturn(Integer.valueOf(2)); + entries[0] = Setup.createAndSetupMemoryImprintEntry(trigger, Result.SUCCESS); + + trigger = mock(GerritTrigger.class); + entries[1] = Setup.createAndSetupMemoryImprintEntryWithEmptyBuild(trigger); + + when(memoryImprint.getEntries()).thenReturn(entries); + + // The only verified value available is of successful build, + // but score should be saturated to Failed verified value from config. + Integer result = instance.getMinimumVerifiedValue(memoryImprint, false, + config.getGerritBuildFailedVerifiedValue()); + assertEquals(Integer.valueOf(config.getGerritBuildFailedVerifiedValue()), result); + } + /** * test. * @throws IOException IOException @@ -467,6 +496,48 @@ public void testGetBuildCompletedCommandMulipleBuildsMessageOrder() throws IOExc Setup.createPatchsetCreated(), -1, 0); } + /** + * Test that verified value will be saturated in case of Failed, but missing built. + */ + @Test + public void testGetBuildCompletedMissingFailedBuild() throws IOException, InterruptedException { + int buildResults = 2; + IGerritHudsonTriggerConfig config = Setup.createConfig(); + Integer expectedVerifiedVote = config.getGerritBuildFailedVerifiedValue(); + Result[] availableResults = new Result[] {Result.SUCCESS, Result.FAILURE}; + + PatchsetCreated event = Setup.createPatchsetCreated(); + TaskListener taskListener = mock(TaskListener.class); + GerritTrigger trigger = mock(GerritTrigger.class); + when(trigger.getGerritBuildSuccessfulVerifiedValue()).thenReturn(null); + when(trigger.getGerritBuildSuccessfulCodeReviewValue()).thenReturn(32); + when(trigger.getCustomUrl()).thenReturn(""); + AbstractProject project = mock(AbstractProject.class); + Setup.setTrigger(trigger, project); + MemoryImprint.Entry[] entries = new MemoryImprint.Entry[buildResults]; + EnvVars env = Setup.createEnvVars(); + + // Remaining successful build. + AbstractBuild build = Setup.createBuild(project, taskListener, env); + env.put("BUILD_URL", jenkins.getRootUrl() + build.getUrl()); + when(build.getResult()).thenReturn(Result.SUCCESS); + when(build.getResult()).thenReturn(Result.SUCCESS); + entries[0] = Setup.createImprintEntry(project, build); + + // Missing failed build. + entries[1] = Setup.createImprintEntry(project, null); + + MemoryImprint memoryImprint = mock(MemoryImprint.class); + when(memoryImprint.getEvent()).thenReturn(event); + when(memoryImprint.getEntries()).thenReturn(entries); + when(memoryImprint.wereAllBuildsSuccessful()).thenReturn(allAreOfType(Result.SUCCESS, availableResults)); + when(memoryImprint.wereAnyBuildsFailed()).thenReturn(anyIsOfType(Result.FAILURE, availableResults)); + ParameterExpander instance = new ParameterExpander(config, jenkins); + String result = instance.getBuildCompletedCommand(memoryImprint, taskListener, null); + + assertThat("Missing VERIFIED", result, containsString("VERIFIED=" + expectedVerifiedVote)); + } + /** * Sub test for {@link #testGetBuildCompletedCommandSuccessful()}. * diff --git a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/mock/Setup.java b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/mock/Setup.java index 335b66dfc..b7b718040 100644 --- a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/mock/Setup.java +++ b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/mock/Setup.java @@ -758,6 +758,12 @@ public static MemoryImprint.Entry createAndSetupMemoryImprintEntry(GerritTrigger return createImprintEntry(project, build); } + public static MemoryImprint.Entry createAndSetupMemoryImprintEntryWithEmptyBuild(GerritTrigger trigger) { + AbstractProject project = mock(AbstractProject.class); + setTrigger(trigger, project); + return createImprintEntry(project, null); + } + /** * Set/mock the supplied trigger onto the supplied {@link AbstractProject} instance. * @param trigger The trigger. From 8364ddf196c13e387eed13458c4f284483cd6d68 Mon Sep 17 00:00:00 2001 From: Josef Prochazka Date: Tue, 18 Jul 2023 18:16:49 +0200 Subject: [PATCH 3/3] Fix formating issues. --- .../trigger/gerritnotifier/ParameterExpander.java | 12 +++++------- .../gerritnotifier/ParameterExpanderTest.java | 15 ++++----------- .../hudson/plugins/gerrit/trigger/mock/Setup.java | 6 ++++++ 3 files changed, 15 insertions(+), 18 deletions(-) diff --git a/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpander.java b/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpander.java index 2eb5f1faf..45e36bc0e 100644 --- a/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpander.java +++ b/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpander.java @@ -388,7 +388,8 @@ protected Integer getVerifiedValue(Result res, GerritTrigger trigger) { * @return the lowest verified value. */ @CheckForNull - public Integer getMinimumVerifiedValue(MemoryImprint memoryImprint, boolean onlyBuilt, Integer maxAllowedVerifiedValue) { + public Integer getMinimumVerifiedValue(MemoryImprint memoryImprint, boolean onlyBuilt, + Integer maxAllowedVerifiedValue) { Integer verified = Integer.MAX_VALUE; for (Entry entry : memoryImprint.getEntries()) { if (entry == null) { @@ -533,19 +534,13 @@ public String getBuildCompletedCommand(MemoryImprint memoryImprint, TaskListener // builds were successful, unstable or failed, we find the minimum // verified/code review value for the NOT_BUILT ones too. boolean onlyCountBuilt = true; - // If some builds failed, but their verified score is no longer - // available, then Successful builds would bump up the score. - // Set upper boundary on verified values based on builds status. - // JENKINS-66535 Integer maxAllowedVerifiedValue = Integer.MAX_VALUE; if (memoryImprint.wereAllBuildsSuccessful()) { command = config.getGerritCmdBuildSuccessful(); } else if (memoryImprint.wereAnyBuildsFailed()) { command = config.getGerritCmdBuildFailed(); - maxAllowedVerifiedValue = config.getGerritBuildFailedVerifiedValue(); } else if (memoryImprint.wereAnyBuildsUnstable()) { command = config.getGerritCmdBuildUnstable(); - maxAllowedVerifiedValue = config.getGerritBuildUnstableVerifiedValue(); } else if (memoryImprint.wereAllBuildsNotBuilt()) { onlyCountBuilt = false; command = config.getGerritCmdBuildNotBuilt(); @@ -554,6 +549,9 @@ public String getBuildCompletedCommand(MemoryImprint memoryImprint, TaskListener } else { //Just as bad as failed for now. command = config.getGerritCmdBuildFailed(); + // Some builds could have failed, but are already deleted and not + // available for score calculation. + // Set pessimistic upper boundary on verified value. maxAllowedVerifiedValue = config.getGerritBuildFailedVerifiedValue(); } diff --git a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderTest.java b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderTest.java index 4e1b6a1d8..43ea014a4 100644 --- a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderTest.java +++ b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderTest.java @@ -497,21 +497,18 @@ public void testGetBuildCompletedCommandMulipleBuildsMessageOrder() throws IOExc } /** - * Test that verified value will be saturated in case of Failed, but missing built. + * Test that verified value will be saturated in case of missing built. + * (Conservative approach is to assume Failed build if the build is now missing.) */ @Test public void testGetBuildCompletedMissingFailedBuild() throws IOException, InterruptedException { int buildResults = 2; IGerritHudsonTriggerConfig config = Setup.createConfig(); Integer expectedVerifiedVote = config.getGerritBuildFailedVerifiedValue(); - Result[] availableResults = new Result[] {Result.SUCCESS, Result.FAILURE}; PatchsetCreated event = Setup.createPatchsetCreated(); TaskListener taskListener = mock(TaskListener.class); GerritTrigger trigger = mock(GerritTrigger.class); - when(trigger.getGerritBuildSuccessfulVerifiedValue()).thenReturn(null); - when(trigger.getGerritBuildSuccessfulCodeReviewValue()).thenReturn(32); - when(trigger.getCustomUrl()).thenReturn(""); AbstractProject project = mock(AbstractProject.class); Setup.setTrigger(trigger, project); MemoryImprint.Entry[] entries = new MemoryImprint.Entry[buildResults]; @@ -519,19 +516,15 @@ public void testGetBuildCompletedMissingFailedBuild() throws IOException, Interr // Remaining successful build. AbstractBuild build = Setup.createBuild(project, taskListener, env); - env.put("BUILD_URL", jenkins.getRootUrl() + build.getUrl()); - when(build.getResult()).thenReturn(Result.SUCCESS); when(build.getResult()).thenReturn(Result.SUCCESS); entries[0] = Setup.createImprintEntry(project, build); - - // Missing failed build. + // Missing build. Possibly Failed, but impossible to know if it is no longer available. entries[1] = Setup.createImprintEntry(project, null); MemoryImprint memoryImprint = mock(MemoryImprint.class); when(memoryImprint.getEvent()).thenReturn(event); when(memoryImprint.getEntries()).thenReturn(entries); - when(memoryImprint.wereAllBuildsSuccessful()).thenReturn(allAreOfType(Result.SUCCESS, availableResults)); - when(memoryImprint.wereAnyBuildsFailed()).thenReturn(anyIsOfType(Result.FAILURE, availableResults)); + ParameterExpander instance = new ParameterExpander(config, jenkins); String result = instance.getBuildCompletedCommand(memoryImprint, taskListener, null); diff --git a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/mock/Setup.java b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/mock/Setup.java index b7b718040..738fe9cee 100644 --- a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/mock/Setup.java +++ b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/mock/Setup.java @@ -758,6 +758,12 @@ public static MemoryImprint.Entry createAndSetupMemoryImprintEntry(GerritTrigger return createImprintEntry(project, build); } + /** + * Create an MemoryImprint.Entry with a given trigger and a null build (missing). + * + * @param trigger the trigger + * @return an entry with the parameters. + */ public static MemoryImprint.Entry createAndSetupMemoryImprintEntryWithEmptyBuild(GerritTrigger trigger) { AbstractProject project = mock(AbstractProject.class); setTrigger(trigger, project);