Repository navigation
fix: removing Thread.sleep() from KubernetesReconcilerCreatorTest - #4915
sarveshkaushal wants to merge 1 commit into
Conversation
|
/assign @brendandburns |
|
As with the other ones, please try to minimize the diff for easier reviewing. |
|
@brendandburns - Formatting changes in this are expected as I have enclosed the code in |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sarveshkaushal The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| @SpringBootTest(classes = {KubernetesReconcilerCreatorTest.App.class}) | ||
| class KubernetesReconcilerCreatorTest { | ||
|
|
||
| private static final Semaphore REQUEST_ENQUEUED = new Semaphore(0); |
There was a problem hiding this comment.
Please don't use globals that aren't constants in the test since it makes it impossible to run tests in parallel in the same JVM.
|
This change looks ok, except for the global Semaphore part, please switch that to be a test local semaphore instead. Also not sure why you added the shutdown (and thus the try/catch) it's not necessarily bad, but I'm not sure its needed. |
11235db to
669e4ae
Compare
Notes
Replaces the fixed
Thread.sleep()inKubernetesReconcilerCreatorTestwith semaphore based synchronization.The custom work-queue key function releases a semaphore after adding the generated request to the controller work queue. The test waits for that signal before asserting on the queue.
This is related to issue #1223
Testing
Ran
mvn installon the project root.