diff --git a/github-pullrequest-plugin/src/main/java/com/github/kostyasha/github/integration/branch/trigger/JobRunnerForBranchCause.java b/github-pullrequest-plugin/src/main/java/com/github/kostyasha/github/integration/branch/trigger/JobRunnerForBranchCause.java index d297420c..046b1663 100644 --- a/github-pullrequest-plugin/src/main/java/com/github/kostyasha/github/integration/branch/trigger/JobRunnerForBranchCause.java +++ b/github-pullrequest-plugin/src/main/java/com/github/kostyasha/github/integration/branch/trigger/JobRunnerForBranchCause.java @@ -65,7 +65,12 @@ public boolean apply(GitHubBranchCause cause) { QueueTaskFuture queueTaskFuture = startJob(cause); if (isNull(queueTaskFuture)) { - LOGGER.error("{} job didn't start", job.getFullName()); + // See JobRunnerForCause: the schedule was refused (job not buildable, or vetoed + // by a Queue.QueueDecisionHandler), so no run will ever publish a final commit + // status and the PENDING pre-status below would stick on the commit forever. + LOGGER.error("{} job didn't start, skipping pending status for {}", + job.getFullName(), cause.getCommitSha()); + return false; } LOGGER.info(sb.toString()); diff --git a/github-pullrequest-plugin/src/main/java/org/jenkinsci/plugins/github/pullrequest/trigger/JobRunnerForCause.java b/github-pullrequest-plugin/src/main/java/org/jenkinsci/plugins/github/pullrequest/trigger/JobRunnerForCause.java index 1ce36eda..2852a264 100644 --- a/github-pullrequest-plugin/src/main/java/org/jenkinsci/plugins/github/pullrequest/trigger/JobRunnerForCause.java +++ b/github-pullrequest-plugin/src/main/java/org/jenkinsci/plugins/github/pullrequest/trigger/JobRunnerForCause.java @@ -101,7 +101,17 @@ public boolean apply(final GitHubPRCause cause) { QueueTaskFuture queueTaskFuture = startJob(cause); if (isNull(queueTaskFuture)) { - LOGGER.error("{} job didn't start", job.getFullName()); + // ParameterizedJobMixIn.scheduleBuild2() returned null, which means the job is + // not buildable (disabled) or a Queue.QueueDecisionHandler vetoed the schedule. + // This is NOT the "identical item already queued" case: Queue.schedule2() returns + // the existing item there, so the future is non null and a run does happen. + // Here no run will ever exist to publish a final commit status, so the PENDING + // pre-status below must not be published. GitHub keeps only the newest status per + // context, so an orphan PENDING pins a required check to "pending" forever and + // permanently blocks the pull request. + LOGGER.error("{} job didn't start, skipping pending status for {}", + job.getFullName(), cause.getHeadSha()); + return false; } LOGGER.info(sb.toString()); diff --git a/github-pullrequest-plugin/src/test/java/com/github/kostyasha/github/integration/branch/trigger/PreStatusJobRunnerForBranchCauseTest.java b/github-pullrequest-plugin/src/test/java/com/github/kostyasha/github/integration/branch/trigger/PreStatusJobRunnerForBranchCauseTest.java new file mode 100644 index 00000000..b4aa3f46 --- /dev/null +++ b/github-pullrequest-plugin/src/test/java/com/github/kostyasha/github/integration/branch/trigger/PreStatusJobRunnerForBranchCauseTest.java @@ -0,0 +1,110 @@ +package com.github.kostyasha.github.integration.branch.trigger; + +import com.coravy.hudson.plugins.github.GithubProjectProperty; +import com.github.kostyasha.github.integration.branch.GitHubBranchCause; +import com.github.kostyasha.github.integration.branch.GitHubBranchPollingLogAction; +import com.github.kostyasha.github.integration.branch.GitHubBranchTrigger; +import hudson.model.FreeStyleProject; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.JenkinsRule; +import org.kohsuke.github.GHCommitState; +import org.kohsuke.github.GHRepository; + +import java.net.URL; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.apache.commons.io.FileUtils.writeStringToFile; +import static org.hamcrest.core.Is.is; +import static org.junit.Assert.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +/** + * Branch counterpart of + * {@code org.jenkinsci.plugins.github.pullrequest.trigger.PreStatusJobRunnerForCauseTest}: the + * PENDING pre-status may only be published when a run was really scheduled, otherwise nothing ever + * overwrites it on the commit. + * + * @see JobRunnerForBranchCause#apply(GitHubBranchCause) + */ +public class PreStatusJobRunnerForBranchCauseTest { + private static final String COMMIT_SHA = "6dcb09b5b57875f334f61aebed695e2e4193db5e"; + private static final String BRANCH = "master"; + + @Rule + public JenkinsRule j = new JenkinsRule(); + + private GHRepository remoteRepo; + + @Before + public void setUp() { + remoteRepo = mock(GHRepository.class); + } + + @Test + public void shouldNotPublishPendingStatusWhenJobDidNotStart() throws Exception { + FreeStyleProject job = j.createFreeStyleProject("disabled-project"); + job.addProperty(new GithubProjectProperty("https://github.com/org/repo")); + // makes isBuildable() false, so ParameterizedJobMixIn.scheduleBuild2() returns null + job.disable(); + + GitHubBranchTrigger trigger = preStatusTrigger(job); + + assertThat("cause shouldn't be reported as triggered", + new JobRunnerForBranchCause(job, trigger).apply(cause()), is(false)); + + verifyNoInteractions(remoteRepo); + assertThat("nothing should be queued", j.jenkins.getQueue().getItems().length, is(0)); + } + + /** + * Guards the assertion above: the very same setup does publish PENDING once a run is scheduled. + */ + @Test + public void shouldPublishPendingStatusWhenJobStarted() throws Exception { + FreeStyleProject job = j.createFreeStyleProject("enabled-project"); + job.addProperty(new GithubProjectProperty("https://github.com/org/repo")); + // keep the run in the queue so it can't finish and publish a final status under us + job.setQuietPeriod(1000); + + GitHubBranchTrigger trigger = preStatusTrigger(job); + + try { + assertThat("cause should be reported as triggered", + new JobRunnerForBranchCause(job, trigger).apply(cause()), is(true)); + + verify(remoteRepo).createCommitStatus(eq(COMMIT_SHA), eq(GHCommitState.PENDING), + any(), anyString(), eq(job.getFullName())); + } finally { + j.jenkins.getQueue().clear(); + } + } + + private GitHubBranchTrigger preStatusTrigger(FreeStyleProject job) throws Exception { + GitHubBranchPollingLogAction logAction = new GitHubBranchPollingLogAction(job); + // apply() reads the polling log into the cause, an absent file would make it return false + // for the wrong reason (IOException) + writeStringToFile(logAction.getPollingLogFile(), "", UTF_8); + + GitHubBranchTrigger trigger = mock(GitHubBranchTrigger.class); + when(trigger.getPollingLogAction()).thenReturn(logAction); + when(trigger.isPreStatus()).thenReturn(true); + when(trigger.getRemoteRepository()).thenReturn(remoteRepo); + return trigger; + } + + private static GitHubBranchCause cause() throws Exception { + GitHubBranchCause cause = new GitHubBranchCause(BRANCH, COMMIT_SHA); + // GitHubBranchBadgeAction dereferences it while scheduling + cause.withHtmlUrl(new URL("https://github.com/org/repo/tree/" + BRANCH)); + cause.withReason("test"); + return cause; + } +} diff --git a/github-pullrequest-plugin/src/test/java/org/jenkinsci/plugins/github/pullrequest/trigger/PreStatusJobRunnerForCauseTest.java b/github-pullrequest-plugin/src/test/java/org/jenkinsci/plugins/github/pullrequest/trigger/PreStatusJobRunnerForCauseTest.java new file mode 100644 index 00000000..05bda4f8 --- /dev/null +++ b/github-pullrequest-plugin/src/test/java/org/jenkinsci/plugins/github/pullrequest/trigger/PreStatusJobRunnerForCauseTest.java @@ -0,0 +1,106 @@ +package org.jenkinsci.plugins.github.pullrequest.trigger; + +import com.coravy.hudson.plugins.github.GithubProjectProperty; +import hudson.model.FreeStyleProject; +import org.jenkinsci.plugins.github.pullrequest.GitHubPRCause; +import org.jenkinsci.plugins.github.pullrequest.GitHubPRPollingLogAction; +import org.jenkinsci.plugins.github.pullrequest.GitHubPRTrigger; +import org.junit.Before; +import org.junit.Test; +import org.kohsuke.github.GHCommitState; +import org.kohsuke.github.GHRepository; + +import java.net.URL; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.apache.commons.io.FileUtils.writeStringToFile; +import static org.hamcrest.core.Is.is; +import static org.jenkinsci.plugins.github.pullrequest.GitHubPRCause.newGitHubPRCause; +import static org.junit.Assert.assertThat; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +/** + * The PENDING pre-status may only be published when a run was really scheduled. Without a run + * nothing ever overwrites it and, because GitHub keeps only the newest status per context, the + * commit stays pinned to "pending" forever. + * + * @see JobRunnerForCause#apply(GitHubPRCause) + */ +public class PreStatusJobRunnerForCauseTest extends JobRunnerForCauseTest { + private static final String HEAD_SHA = "6dcb09b5b57875f334f61aebed695e2e4193db5e"; + + private GHRepository remoteRepo; + + @Before + public void setUp() { + remoteRepo = mock(GHRepository.class); + } + + @Test + public void shouldNotPublishPendingStatusWhenJobDidNotStart() throws Exception { + FreeStyleProject job = j.createFreeStyleProject("disabled-project"); + job.addProperty(new GithubProjectProperty("https://github.com/org/repo")); + // makes isBuildable() false, so ParameterizedJobMixIn.scheduleBuild2() returns null + job.disable(); + + GitHubPRTrigger trigger = preStatusTrigger(job); + + assertThat("cause shouldn't be reported as triggered", + new JobRunnerForCause(job, trigger).apply(cause()), is(false)); + + verifyNoInteractions(remoteRepo); + assertThat("nothing should be queued", j.jenkins.getQueue().getItems().length, is(0)); + } + + /** + * Guards the assertion above: the very same setup does publish PENDING once a run is scheduled, + * so the test can't pass just because the status call was broken. + */ + @Test + public void shouldPublishPendingStatusWhenJobStarted() throws Exception { + FreeStyleProject job = j.createFreeStyleProject("enabled-project"); + job.addProperty(new GithubProjectProperty("https://github.com/org/repo")); + // keep the run in the queue so it can't finish and publish a final status under us + job.setQuietPeriod(1000); + + GitHubPRTrigger trigger = preStatusTrigger(job); + + try { + assertThat("cause should be reported as triggered", + new JobRunnerForCause(job, trigger).apply(cause()), is(true)); + + verify(remoteRepo).createCommitStatus(eq(HEAD_SHA), eq(GHCommitState.PENDING), + anyString(), anyString(), eq(job.getFullName())); + } finally { + j.jenkins.getQueue().clear(); + } + } + + private GitHubPRTrigger preStatusTrigger(FreeStyleProject job) throws Exception { + GitHubPRPollingLogAction logAction = new GitHubPRPollingLogAction(job); + // apply() reads the polling log into the cause, an absent file would make it return false + // for the wrong reason (IOException) + writeStringToFile(logAction.getPollingLogFile(), "", UTF_8); + + GitHubPRTrigger trigger = mock(GitHubPRTrigger.class); + when(trigger.getPollingLogAction()).thenReturn(logAction); + when(trigger.isPreStatus()).thenReturn(true); + when(trigger.getRemoteRepository()).thenReturn(remoteRepo); + return trigger; + } + + private static GitHubPRCause cause() throws Exception { + GitHubPRCause cause = newGitHubPRCause() + .withNumber(10) + .withHeadSha(HEAD_SHA); + // GitHubPREnv.URL dereferences it while filling build parameters + cause.withHtmlUrl(new URL("https://github.com/org/repo/pull/10")); + cause.withReason("test"); + return cause; + } +}