JENKINS-63760 notifyQueued used #70
Conversation
dwnusbaum
left a comment
dwnusbaum
left a comment
There was a problem hiding this comment.
There are some tests failing. I think one of the failures is because the Jenkinsfile needs to be updated to remove buildPlugin.recommendedConfigurations because the recommended versions are too old for the current baseline of the plugin (that will need to be done by a maintainer with write access).
| null); | ||
| executorService.submit(() -> { | ||
| QueueItemMetricsListener.notifyStarted(m); | ||
| QueueItemMetricsListener.notifyQueued(m); |
There was a problem hiding this comment.
Looks like you accidentally added a 5th space:
| QueueItemMetricsListener.notifyQueued(m); | |
| QueueItemMetricsListener.notifyQueued(m); |
|
#72 got merged. Now the failures should be genuine, if any. |
dwnusbaum
left a comment
dwnusbaum
left a comment
There was a problem hiding this comment.
Looks good to me, here is a link to the Jira ticket for reference that has some details from when I was filed the issue: JENKINS-63760.
| null); | ||
| executorService.submit(() -> { | ||
| QueueItemMetricsListener.notifyStarted(m); | ||
| QueueItemMetricsListener.notifyQueued(m); |
There was a problem hiding this comment.
This has the potential to be a breaking change, but from a quick GitHub search it looks like there aren't any open source plugins using QueueItemMetricsListener in the first place, so the risk seems limited.
| Thread.sleep(10); | ||
| } | ||
|
|
||
| assertThat(listener.state.toString(), containsString("Q")); |
There was a problem hiding this comment.
Could you please assert with equal? It leaves the test more comprehensive and avoid failures in case we add some new state on the future with this letter. The same for the other cases.
MRamonLeon
left a comment
MRamonLeon
left a comment
There was a problem hiding this comment.
Sorry, I didn't realize this texts where added by you
5 participants
notifyQueued wasn't used at all.
Also added tests for completion.