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

PublishChecks step: The conclusion should be none when status is not completed #82

Merged
merged 5 commits into from
Feb 18, 2021
Merged

PublishChecks step: The conclusion should be none when status is not completed #82

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
206f586
Changed conclusion status based on ChecksStatus
ThusithaDJ Feb 10, 2021
4b9b748
Refactoring the implementation
ThusithaDJ Feb 13, 2021
263e404
Fix PMD issues.
ThusithaDJ Feb 13, 2021
3f39628
Update src/main/java/io/jenkins/plugins/checks/steps/PublishChecksSte…
ThusithaDJ Feb 17, 2021
b3c4522
Refactor unit tests
ThusithaDJ Feb 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/main/java/io/jenkins/plugins/checks/steps/PublishChecksStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,10 @@ static class PublishChecksStepExecution extends SynchronousNonBlockingStepExecut
PublishChecksStepExecution(final StepContext context, final PublishChecksStep step) {
super(context);
this.step = step;
if ((step.getConclusion() == ChecksConclusion.SUCCESS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a rule on Github checks API that if the user is providing the conclusion, then the status should be automatically set as completed. And because most of our users are GitHub users and will most likely refer to this, so it would be strange for them if the conclusion is reset to none (although it's a misuse to set the status not as completed while setting the conclusion to be a success).

So I would move this logic to the setters. If the user calls the methods and sets the status to be queued or in_progress, we just change the conclusion to be none; if the user sets the conclusion, we change the status to be completed again.

Copy link
Contributor Author

@ThusithaDJ ThusithaDJ Feb 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sure, @XiongKezhi. But based on your comment, what if the user sets the status as queued or in_progress and provides a conclusion? Conclusion should be whatever the value provided, and status should be completed, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation should handle that, like GitHub will automatically set the status to be completed if the conclusion is provided. We just need to make the step easier to use IMO: they don't have to explicitly set the conclusion to be none.

&& (step.getStatus() == ChecksStatus.QUEUED || step.getStatus() == ChecksStatus.IN_PROGRESS)) {
this.step.setConclusion(ChecksConclusion.NONE);
}
}

@Override
Expand Down
62 changes: 62 additions & 0 deletions src/test/java/io/jenkins/plugins/checks/steps/PublishChecksStepTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,68 @@ void shouldPublishCheckWithDefaultValues() throws IOException, InterruptedExcept
.build());
}

@Test
void shouldPublishCheckWithStatusInProgress() throws IOException, InterruptedException {
PublishChecksStep step = new PublishChecksStep();
step.setName("Jenkins");
step.setSummary("a check made by Jenkins");
step.setTitle("Jenkins Build");
step.setText("an in progress build");
step.setStatus(ChecksStatus.IN_PROGRESS);
step.setDetailsURL("http://ci.jenkins.io");

StepContext context = mock(StepContext.class);
when(context.get(Run.class)).thenReturn(mock(Run.class));
when(context.get(TaskListener.class)).thenReturn(TaskListener.NULL);

StepExecution execution = step.start(context);
assertThat(execution).isInstanceOf(PublishChecksStep.PublishChecksStepExecution.class);
ThusithaDJ marked this conversation as resolved.
Show resolved Hide resolved
assertThat(((PublishChecksStep.PublishChecksStepExecution)execution).extractChecksDetails())
.usingRecursiveComparison()
.isEqualTo(new ChecksDetails.ChecksDetailsBuilder()
.withName("Jenkins")
.withStatus(ChecksStatus.IN_PROGRESS)
.withConclusion(ChecksConclusion.NONE)
.withDetailsURL("http://ci.jenkins.io")
.withOutput(new ChecksOutput.ChecksOutputBuilder()
.withTitle("Jenkins Build")
.withSummary("a check made by Jenkins")
.withText("an in progress build")
.build())
.build());
}

@Test
void shouldPublishCheckWithStatusQueue() throws IOException, InterruptedException {
PublishChecksStep step = new PublishChecksStep();
step.setName("Jenkins");
step.setSummary("a check made by Jenkins");
step.setTitle("Jenkins Build");
step.setText("a queued build");
step.setStatus(ChecksStatus.QUEUED);
step.setDetailsURL("http://ci.jenkins.io");

StepContext context = mock(StepContext.class);
when(context.get(Run.class)).thenReturn(mock(Run.class));
when(context.get(TaskListener.class)).thenReturn(TaskListener.NULL);

StepExecution execution = step.start(context);
assertThat(execution).isInstanceOf(PublishChecksStep.PublishChecksStepExecution.class);
assertThat(((PublishChecksStep.PublishChecksStepExecution)execution).extractChecksDetails())
.usingRecursiveComparison()
.isEqualTo(new ChecksDetails.ChecksDetailsBuilder()
.withName("Jenkins")
.withStatus(ChecksStatus.QUEUED)
.withConclusion(ChecksConclusion.NONE)
.withDetailsURL("http://ci.jenkins.io")
.withOutput(new ChecksOutput.ChecksOutputBuilder()
.withTitle("Jenkins Build")
.withSummary("a check made by Jenkins")
.withText("a queued build")
.build())
.build());
}

@Test
void shouldPublishCheckWithSetValues() throws IOException, InterruptedException {
PublishChecksStep step = new PublishChecksStep();
Expand Down