PublishChecks step: The conclusion should be none when status is not completed #82
Conversation
Codecov Report
@@ Coverage Diff @@
## master #82 +/- ##
============================================
- Coverage 87.31% 86.82% -0.50%
- Complexity 139 140 +1
============================================
Files 16 16
Lines 623 630 +7
Branches 47 49 +2
============================================
+ Hits 544 547 +3
- Misses 63 65 +2
- Partials 16 18 +2
Continue to review full report at Codecov.
|
| @@ -169,6 +169,10 @@ private String asDisplayName(final String name) { | |||
| PublishChecksStepExecution(final StepContext context, final PublishChecksStep step) { | |||
| super(context); | |||
| this.step = step; | |||
| if ((step.getConclusion() == ChecksConclusion.SUCCESS) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
src/test/java/io/jenkins/plugins/checks/steps/PublishChecksStepTest.java
Show resolved
Hide resolved
|
Hi @XiongKezhi, if you don't mind, could you please check and let me know your thoughts about these changes and how can I get rid of these failed statuses. |
src/main/java/io/jenkins/plugins/checks/steps/PublishChecksStep.java
Outdated
Show resolved
Hide resolved
| step.setDetailsURL("http://ci.jenkins.io"); | ||
| void shouldPublishCheckWithStatusInProgress() throws IOException, InterruptedException { | ||
| PublishChecksStep step = getModifiedPublishChecksStepObject("an in progress build", | ||
| ChecksStatus.IN_PROGRESS,null); |
There was a problem hiding this comment.
| ChecksStatus.IN_PROGRESS,null); | |
| ChecksStatus.IN_PROGRESS, null); |
Same for the next check style warning
|
|
||
| @BeforeEach | ||
| void setup() throws IOException, InterruptedException { |
There was a problem hiding this comment.
In this project, if possible, I normally avoid using @BeforeEach and shared variables between tests. I usually just make a private method like getModifiedPublishChecksStepObject that returns what I want.
…p.java Co-authored-by: Kezhi Xiong <august.xkz@gmail.com>
No description provided.