Listen to checkout instead of on started. #13
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13 +/- ##
============================================
+ Coverage 82.54% 83.33% +0.78%
Complexity 49 49
============================================
Files 10 10
Lines 212 210 -2
Branches 7 6 -1
============================================
Hits 175 175
+ Misses 37 35 -2
Continue to review full report at Codecov.
|
src/main/java/io/jenkins/plugins/checks/BuildStatusChecksPublisher.java
Outdated
Show resolved
Hide resolved
| public void onStarted(final Run run, final TaskListener listener) { | ||
| publish(ChecksPublisherFactory.fromRun(run, listener), | ||
| ChecksStatus.IN_PROGRESS, ChecksConclusion.NONE); | ||
| public void onCheckout(final Run<?, ?> run, final SCM scm, final FilePath workspace, |
There was a problem hiding this comment.
Is it a problem if this method will be invoked multiple times in a job? E.g., a job may have several SCM checkouts. I'm not sure how our CI is configured, but from https://ci.jenkins.io/job/Plugins/job/analysis-model/job/master/984/ it looks like we have 2 checkouts for each job.
There was a problem hiding this comment.
is there a way to inject an extension to Jenkins in integration test?
There was a problem hiding this comment.
I wonder why there are two checkouts so that I can reproduce that and test.
There was a problem hiding this comment.
Yes you can have a static inner class annotated with TestExtension.
Our jobs should be just checking out the shared library and the repo.
There was a problem hiding this comment.
I didn't find a better way to test the listeners than adding log information (maybe fine level) and then check the log, but I don't know if this is a good practice to add such log information for tests.
Our jobs should be just checking out the shared library and the repo.
I don't understand this, should we do any other works (check whether the checkout is about the shared library or the repo) in this onCheckout method?
There was a problem hiding this comment.
should be fine.
re: testing,
I wouldn't worry about testing listeners get invoked that's tested by Jenkins, if there's logic in it you should test that separately by just passing required params and checking it does what is expected
| publish(ChecksPublisherFactory.fromJob((Job)wi.task, null), | ||
| ChecksStatus.QUEUED, ChecksConclusion.NONE); | ||
| publish(ChecksPublisherFactory.fromJob((Job) wi.task, TaskListener.NULL), ChecksStatus.QUEUED, | ||
| ChecksConclusion.NONE); |
There was a problem hiding this comment.
The Codecov annotation is really nice 🥰
I think this is a good topic for a thesis for one of my students that can be based on the new Coverage and Checks API plugins...
Listen to checkout instead of on started can benefit the implementations since
onCheckout, implementations can resolve the commit from the run instead of remote repository.Related jenkinsci/github-checks-plugin#21 and jenkinsci/github-checks-plugin#27