Skip to content

Commit

Permalink
Don't store actions on jobs (#99)
Browse files Browse the repository at this point in the history 
* Don't store actions on jobs

* Run GitHubChecksPublisherITest on freestyle and pipeline jobs

* Make checkstyle happy

* Test jobs and runs

* Unify GitHubSCMSourceChecksContext constructors
  • Loading branch information
@mrginglymus
mrginglymus authored Dec 29, 2020
1 parent f9c6e96 commit ee26572
Show file tree
Hide file tree
Showing 6 changed files with 147 additions and 57 deletions.
12 changes: 10 additions & 2 deletions src/main/java/io/jenkins/plugins/checks/github/GitHubChecksContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import edu.hm.hafner.util.FilteredLog;
import edu.umd.cs.findbugs.annotations.CheckForNull;
import hudson.model.Job;
import hudson.model.Run;
import org.apache.commons.lang3.StringUtils;
import org.jenkinsci.plugins.github_branch_source.GitHubAppCredentials;

Expand Down Expand Up @@ -113,18 +114,25 @@ public Optional<Long> getId(final String name) {
return getAction(name).map(GitHubChecksAction::getId);
}

protected abstract Optional<Run<?, ?>> getRun();

private Optional<GitHubChecksAction> getAction(final String name) {
return job.getActions(GitHubChecksAction.class)
if (!getRun().isPresent()) {
return Optional.empty();
}
return getRun().get().getActions(GitHubChecksAction.class)
.stream()
.filter(a -> a.getName().equals(name))
.findFirst();
}

void addActionIfMissing(final long id, final String name) {
if (!getRun().isPresent()) {
return;
}
Optional<GitHubChecksAction> action = getAction(name);
if (!action.isPresent()) {
job.addAction(new GitHubChecksAction(id, name));
getRun().get().addAction(new GitHubChecksAction(id, name));
}
}
}
4 changes: 2 additions & 2 deletions src/main/java/io/jenkins/plugins/checks/github/GitHubChecksPublisherFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,13 @@ public GitHubChecksPublisherFactory() {
@Override
protected Optional<ChecksPublisher> createPublisher(final Run<?, ?> run, final TaskListener listener) {
final String runURL = urlProvider.getRunURL(run);
return createPublisher(listener, new GitHubSCMSourceChecksContext(run, runURL, scmFacade),
return createPublisher(listener, GitHubSCMSourceChecksContext.fromRun(run, runURL, scmFacade),
new GitSCMChecksContext(run, runURL, scmFacade));
}

@Override
protected Optional<ChecksPublisher> createPublisher(final Job<?, ?> job, final TaskListener listener) {
return createPublisher(listener, new GitHubSCMSourceChecksContext(job, urlProvider.getJobURL(job), scmFacade));
return createPublisher(listener, GitHubSCMSourceChecksContext.fromJob(job, urlProvider.getJobURL(job), scmFacade));
}

private Optional<ChecksPublisher> createPublisher(final TaskListener listener, final GitHubChecksContext... contexts) {
Expand Down
45 changes: 24 additions & 21 deletions src/main/java/io/jenkins/plugins/checks/github/GitHubSCMSourceChecksContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,35 +17,33 @@
class GitHubSCMSourceChecksContext extends GitHubChecksContext {
@CheckForNull
private final String sha;
@CheckForNull
private final Run<?, ?> run;

/**
* Creates a {@link GitHubSCMSourceChecksContext} according to the run. All attributes are computed during this period.
*
* @param run
* a run of a GitHub Branch Source project
* @param runURL
* the URL to the Jenkins run
* @param scmFacade
* a facade for Jenkins SCM
*/
GitHubSCMSourceChecksContext(final Run<?, ?> run, final String runURL, final SCMFacade scmFacade) {
super(run.getParent(), runURL, scmFacade);
sha = resolveHeadSha(run);
static GitHubSCMSourceChecksContext fromRun(final Run<?, ?> run, final String runURL, final SCMFacade scmFacade) {
return new GitHubSCMSourceChecksContext(run.getParent(), run, runURL, scmFacade);
}

static GitHubSCMSourceChecksContext fromJob(final Job<?, ?> job, final String runURL, final SCMFacade scmFacade) {
return new GitHubSCMSourceChecksContext(job, null, runURL, scmFacade);
}

/**
* Creates a {@link GitHubSCMSourceChecksContext} according to the job. All attributes are computed during this period.
* Creates a {@link GitHubSCMSourceChecksContext} according to the job and run, if provided. All attributes are computed during this period.
*
* @param job
* a GitHub Branch Source project
* @param jobURL
* the URL to the Jenkins job
* @param run
* a run of a GitHub Branch Source project
* @param runURL
* the URL to the Jenkins run
* @param scmFacade
* a facade for Jenkins SCM
*/
GitHubSCMSourceChecksContext(final Job<?, ?> job, final String jobURL, final SCMFacade scmFacade) {
super(job, jobURL, scmFacade);
sha = resolveHeadSha(job);
private GitHubSCMSourceChecksContext(final Job<?, ?> job, @CheckForNull final Run<?, ?> run, final String runURL, final SCMFacade scmFacade) {
super(job, runURL, scmFacade);
this.run = run;
this.sha = Optional.ofNullable(run).map(this::resolveHeadSha).orElse(resolveHeadSha(job));
}

@Override
Expand Down Expand Up @@ -91,6 +89,11 @@ public boolean isValid(final FilteredLog logger) {
return true;
}

@Override
protected Optional<Run<?, ?>> getRun() {
return Optional.ofNullable(run);
}

@Override
@CheckForNull
protected String getCredentialsId() {
Expand All @@ -108,10 +111,10 @@ private GitHubSCMSource resolveSource() {
}

@CheckForNull
private String resolveHeadSha(final Run<?, ?> run) {
private String resolveHeadSha(final Run<?, ?> theRun) {
GitHubSCMSource source = resolveSource();
if (source != null) {
Optional<SCMRevision> revision = getScmFacade().findRevision(source, run);
Optional<SCMRevision> revision = getScmFacade().findRevision(source, theRun);
if (revision.isPresent()) {
return getScmFacade().findHash(revision.get()).orElse(null);
}
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/io/jenkins/plugins/checks/github/GitSCMChecksContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ class GitSCMChecksContext extends GitHubChecksContext {
this.run = run;
}

@Override
protected Optional<Run<?, ?>> getRun() {
return Optional.of(run);
}

@Override
public String getHeadSha() {
try {
Expand Down
106 changes: 90 additions & 16 deletions src/test/java/io/jenkins/plugins/checks/github/GitHubChecksPublisherITest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
import java.time.LocalDateTime;
import java.time.ZoneOffset;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.Optional;
import java.util.function.Function;
import java.util.logging.Level;

import com.fasterxml.jackson.core.JsonProcessingException;
Expand All @@ -15,13 +17,19 @@
import com.fasterxml.jackson.databind.ObjectReader;
import com.fasterxml.jackson.databind.PropertyNamingStrategy;
import com.fasterxml.jackson.databind.introspect.VisibilityChecker;
import hudson.model.FreeStyleProject;
import hudson.model.Job;
import hudson.model.Queue;
import io.jenkins.plugins.util.IntegrationTestWithJenkinsPerTest;
import io.jenkins.plugins.util.PluginLogger;
import jenkins.model.ParameterizedJobMixIn;
import org.jenkinsci.plugins.github_branch_source.Connector;
import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.LoggerRule;

Expand Down Expand Up @@ -50,7 +58,6 @@
import org.kohsuke.github.GHRepository;
import org.kohsuke.github.GitHub;
import org.mockito.MockedStatic;
import org.mockito.Mockito;

import static com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility.ANY;
import static com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility.NONE;
Expand All @@ -61,9 +68,45 @@
* Tests if the {@link GitHubChecksPublisher} actually sends out the requests to GitHub in order to publish the check
* runs.
*/
@SuppressWarnings({"PMD.ExcessiveImports", "checkstyle:ClassDataAbstractionCoupling", "rawtypes", "checkstyle:ClassFanOutComplexity"})
@RunWith(Parameterized.class)
@SuppressWarnings({"PMD.ExcessiveImports", "checkstyle:ClassDataAbstractionCoupling", "rawtypes", "checkstyle:ClassFanOutComplexity", "checkstyle:JavaNCSS"})
public class GitHubChecksPublisherITest extends IntegrationTestWithJenkinsPerTest {

/**
* Provides parameters for tests.
* @return A list of methods used to create GitHubChecksContexts, with which each test should be run.
*/
@Parameterized.Parameters(name = "{0}")
public static Collection<Object[]> contextBuilders() {
return Arrays.asList(new Object[][]{
{"Freestyle (run)", (Function<GitHubChecksPublisherITest, GitHubChecksContext>) GitHubChecksPublisherITest::createGitHubChecksContextWithGitHubSCMFreestyle, false},
{"Freestyle (job)", (Function<GitHubChecksPublisherITest, GitHubChecksContext>) GitHubChecksPublisherITest::createGitHubChecksContextWithGitHubSCMFreestyle, true},
{"Pipeline (run)", (Function<GitHubChecksPublisherITest, GitHubChecksContext>) GitHubChecksPublisherITest::createGitHubChecksContextWithGitHubSCMFromPipeline, false},
{"Pipeline (job)", (Function<GitHubChecksPublisherITest, GitHubChecksContext>) GitHubChecksPublisherITest::createGitHubChecksContextWithGitHubSCMFromPipeline, true}
});
}

/**
* Human readable name of the context builder - used only for test name formatting.
*/
@SuppressWarnings("checkstyle:VisibilityModifier")
@Parameterized.Parameter(0)
public String contextBuilderName;

/**
* Reference to method used to create GitHubChecksContext with either a pipeline or freestyle job.
*/
@SuppressWarnings("checkstyle:VisibilityModifier")
@Parameterized.Parameter(1)
public Function<GitHubChecksPublisherITest, GitHubChecksContext> contextBuilder;

/**
* Create GitHubChecksContext from the job instead of the run.
*/
@SuppressWarnings("checkstyle:VisibilityModifier")
@Parameterized.Parameter(2)
public boolean fromJob;

/**
* Rule for the log system.
*/
Expand Down Expand Up @@ -123,7 +166,7 @@ public void shouldPublishGitHubCheckRunCorrectly() {
new ChecksAction("re-run", "re-run Jenkins build", "#0")))
.build();

new GitHubChecksPublisher(createGitHubChecksContextWithGitHubSCM(),
new GitHubChecksPublisher(contextBuilder.apply(this),
new PluginLogger(getJenkins().createTaskListener().getLogger(), "GitHub Checks"),
wireMockRule.baseUrl())
.publish(details);
Expand Down Expand Up @@ -157,7 +200,7 @@ public void shouldLogChecksParametersIfExceptionHappensWhenPublishChecks() {
.build())
.build();

new GitHubChecksPublisher(createGitHubChecksContextWithGitHubSCM(),
new GitHubChecksPublisher(contextBuilder.apply(this),
new PluginLogger(getJenkins().createTaskListener().getLogger(), "GitHub Checks"),
wireMockRule.baseUrl())
.publish(details);
Expand Down Expand Up @@ -230,7 +273,7 @@ public void testChecksPublisherUpdatesCorrectly() throws Exception {
try (MockedStatic<Connector> connector = mockStatic(Connector.class)) {
connector.when(() -> Connector.connect(anyString(), any(GitHubAppCredentials.class))).thenReturn(gitHub);

GitHubChecksContext context = createGitHubChecksContextWithGitHubSCM();
GitHubChecksContext context = contextBuilder.apply(this);

ChecksDetails details1 = new ChecksDetailsBuilder()
.withName(checksName1)
Expand All @@ -251,7 +294,12 @@ public void testChecksPublisherUpdatesCorrectly() throws Exception {
verify(createBuilder2, never()).create();
verify(updateBuilder1, never()).create();

assertThat(context.getId(checksName1)).isPresent().get().isEqualTo(checksId1);
if (fromJob) {
assertThat(context.getId(checksName1)).isNotPresent();
}
else {
assertThat(context.getId(checksName1)).isPresent().get().isEqualTo(checksId1);
}
assertThat(context.getId(checksName2)).isNotPresent();

ChecksDetails details2 = new ChecksDetailsBuilder()
Expand All @@ -266,8 +314,14 @@ public void testChecksPublisherUpdatesCorrectly() throws Exception {
verify(createBuilder2, times(1)).create();
verify(updateBuilder1, never()).create();

assertThat(context.getId(checksName1)).isPresent().get().isEqualTo(checksId1);
assertThat(context.getId(checksName2)).isPresent().get().isEqualTo(checksId2);
if (fromJob) {
assertThat(context.getId(checksName1)).isNotPresent();
assertThat(context.getId(checksName1)).isNotPresent();
}
else {
assertThat(context.getId(checksName1)).isPresent().get().isEqualTo(checksId1);
assertThat(context.getId(checksName2)).isPresent().get().isEqualTo(checksId2);
}

ChecksDetails updateDetails1 = new ChecksDetailsBuilder()
.withName(checksName1)
Expand All @@ -277,19 +331,34 @@ public void testChecksPublisherUpdatesCorrectly() throws Exception {

publisher.publish(updateDetails1);

verify(createBuilder1, times(1)).create();
verify(createBuilder1, times(fromJob ? 2 : 1)).create();
verify(createBuilder2, times(1)).create();
verify(updateBuilder1, times(1)).create();

assertThat(context.getId(checksName1)).isPresent().get().isEqualTo(checksId1);
assertThat(context.getId(checksName2)).isPresent().get().isEqualTo(checksId2);

verify(updateBuilder1, times(fromJob ? 0 : 1)).create();

if (fromJob) {
assertThat(context.getId(checksName1)).isNotPresent();
assertThat(context.getId(checksName1)).isNotPresent();
}
else {
assertThat(context.getId(checksName1)).isPresent().get().isEqualTo(checksId1);
assertThat(context.getId(checksName2)).isPresent().get().isEqualTo(checksId2);
}
}
}

private GitHubChecksContext createGitHubChecksContextWithGitHubSCM() {
private GitHubChecksContext createGitHubChecksContextWithGitHubSCMFreestyle() {
FreeStyleProject job = createFreeStyleProject();
return createGitHubChecksContextWithGitHubSCM(job);
}

private GitHubChecksContext createGitHubChecksContextWithGitHubSCMFromPipeline() {
WorkflowJob job = createPipeline();
job.setDefinition(new CpsFlowDefinition("node {}", true));
return createGitHubChecksContextWithGitHubSCM(job);
}

private <R extends Run<J, R> & Queue.Executable, J extends Job<J, R> & ParameterizedJobMixIn.ParameterizedJob<J, R>>
GitHubChecksContext createGitHubChecksContextWithGitHubSCM(final J job) {
Run run = buildSuccessfully(job);

SCMFacade scmFacade = mock(SCMFacade.class);
Expand All @@ -308,10 +377,15 @@ private GitHubChecksContext createGitHubChecksContextWithGitHubSCM() {
when(scmFacade.findGitHubAppCredentials(job, "1")).thenReturn(Optional.of(credentials));
when(scmFacade.findHead(job)).thenReturn(Optional.of(head));
when(scmFacade.findRevision(source, run)).thenReturn(Optional.of(revision));
when(scmFacade.findRevision(source, head)).thenReturn(Optional.of(revision));
when(scmFacade.findHash(revision)).thenReturn(Optional.of("18c8e2fd86e7aa3748e279c14a00dc3f0b963e7f"));

when(urlProvider.getRunURL(run)).thenReturn("https://ci.jenkins.io");
when(urlProvider.getJobURL(job)).thenReturn("https://ci.jenkins.io");

return new GitHubSCMSourceChecksContext(run, urlProvider.getRunURL(run), scmFacade);
if (fromJob) {
return GitHubSCMSourceChecksContext.fromJob(job, urlProvider.getJobURL(job), scmFacade);
}
return GitHubSCMSourceChecksContext.fromRun(run, urlProvider.getRunURL(run), scmFacade);
}
}
Loading

0 comments on commit ee26572

Please sign in to comment.