[JENKINS-48770] Quiet period is in milliseconds #3591
Conversation
| if (delay==null) delay=new TimeDuration(getJob().getQuietPeriod()); | ||
| if (delay==null) | ||
| // time set in UI is in seconds. Changing it into milliseconds | ||
| delay=new TimeDuration(getJob().getQuietPeriod() * 1000); |
There was a problem hiding this comment.
It may not be super useful here, but when converting time units, I usually like to use the TimeUnit enum to convert. Makes it less necessary to comment on the units since it's obvious based on the code then.
There was a problem hiding this comment.
I'm not used to do it, but I like the idea.
There was a problem hiding this comment.
+1, there're a lot of examples in case you are looking for it in the core @fcojfernandez. https://github.com/jenkinsci/jenkins/search?q=TimeUnit&unscoped_q=TimeUnit
For instance, to convert 5 seconds into milliseconds:
PR lacks both. Given that this was a regression, shouldn't there be tests preventing repeated problems down the road? |
There was a problem hiding this comment.
I agree with others that it would be nice to have tests verifying the correct behavior. I haven't looked at it enough to figure out how feasible that might be or how they might be implemented.
Also, it would be kind of nice to reduce the duplication, perhaps refactoring to create a common method or two. At a minimum perhaps that might be easily testable.
|
@jeffret-b @daniel-beck new test added |
| @Issue("JENKINS-48770") | ||
| public void doBuildQuietPeriodInSeconds() throws Exception { | ||
| final int projectQuietPeriodInSeconds = 10; | ||
| final int projectQuietPeriodInMillis = projectQuietPeriodInSeconds * 1000; |
There was a problem hiding this comment.
Ironically, no use of TimeUnit here.
| Assert.assertEquals(0, project.getBuilds().size()); | ||
| Thread.sleep(projectQuietPeriodInMillis / 2); | ||
| Assert.assertEquals(0, project.getBuilds().size()); | ||
| Thread.sleep(projectQuietPeriodInMillis / 2 + timeToExecuteProject); |
There was a problem hiding this comment.
This test seems really long (execution duration of ~15 seconds) for what it accomplishes. I wonder whether it couldn't be sped up.
There was a problem hiding this comment.
Queue items tell you how long they're in the queue for. So just set the quiet period to 50 (seconds), trigger a build, ask the Queue.WaitingItem#timestamp whether it'll wait for at least another 45000 millis, and then just abort it?
(If it's not a WaitingItem something else will have gone wrong. Typically LeftItem, meaning the build has already started.)
|
@daniel-beck any chance for this PR to be merged or is something else missing before that? |
[JENKINS-48770] Quiet period is in milliseconds (cherry picked from commit c0d9fb7)
See JENKINS-48770.
Detected a regression in how the Quiet Period value from the UI was used. This issue and PR try to fix it.
Proposed changelog entries
Submitter checklist
* Use the
Internal:prefix if the change has no user-visible impact (API, test frameworks, etc.)Desired reviewers
@reviewbybees @oleg-nenashev @daniel-beck @batmat