Skip to content

[Umbrella Issue] Many tests use Thread.sleep instead of synchronization primitives #1223

Open

Description

@brendandburns

Many of our tests simply have a Thread.sleep(duration) when testing multi-threaded code. This is inherently flaky.

Instead we should use synchronization classes (generally Semaphore) to synchronized the tests which is both faster in the general case and not flaky in the extreme case.

See for instance #1218 and #1219

Activity

  1. brendandburns commented on Sep 3, 2020

    @brendandburns
    ContributorAuthor
    • util/src/test/java/io/kubernetes/client/informer/cache/ReflectorRunnableTest.java
    • extended/src/test/java/io/kubernetes/client/extended/workqueue/DefaultDelayingQueueTest.java
    • util/src/test/java/io/kubernetes/client/informer/cache/ProcessorListenerTest.java
    • util/src/test/java/io/kubernetes/client/informer/cache/SharedProcessorTest.java
    • util/src/test/java/io/kubernetes/client/CopyTest.java
    • util/src/test/java/io/kubernetes/client/informer/cache/ControllerTest.java
    • extended/src/test/java/io/kubernetes/client/extended/workqueue/DefaultWorkQueueTest.java
    • extended/src/test/java/io/kubernetes/client/extended/leaderelection/LeaderElectionTest.java
    • util/src/test/java/io/kubernetes/client/informer/impl/DefaultSharedIndexInformerTest.java
    • extended/src/test/java/io/kubernetes/client/extended/controller/LeaderElectingControllerTest.java
    • util/src/test/java/io/kubernetes/client/PortForwardTest.java
    • util/src/test/java/io/kubernetes/client/AttachTest.java
    • util/src/test/java/io/kubernetes/client/ExecTest.java
    • extended/src/test/java/io/kubernetes/client/extended/workqueue/ratelimiter/BucketRateLimiterTest.java
    • spring/src/test/java/io/kubernetes/client/spring/extended/controller/KubernetesInformerCreatorTest.java
    • spring/src/test/java/io/kubernetes/client/spring/extended/controller/KubernetesReconcilerCreatorTest.java
    • extended/src/test/java/io/kubernetes/client/extended/event/EventCorrelatorTest.java
  2. yue9944882 commented on Sep 3, 2020

    @yue9944882
    Member

    @brendandburns thanks for sorting out these potentially flaking tests.. will fix them later!

    /assign

  3. sarveshkaushal commented on Sep 3, 2020

    @sarveshkaushal
    Contributor

    I would like to help :-)

  4. brendandburns commented on Sep 3, 2020

    @brendandburns
    ContributorAuthor

    @sarveshkaushal we'd welcome the help. In most cases the Thread.sleep(foo) is to wait for some other thread to complete it's work. Instead, we should create a Semaphore, then acquire() one or more permits for the executions we're waiting for, and then release() them in the Thread, then attempt to acquire() in the test thread to implement the wait.

    See an example PR #1219

    The reason that Thread.sleep() is flaky is because in a CI/CD system like GitHub actions the machines can be massively overloaded, which leads to much longer delays than you might otherwise see when running tests, so the sleep(..) ends up not being long enough. You can always make the sleep longer, but that makes the tests run longer (and it's still flaky in extreme cases) so using synchronization primitives is (almost) always the right answer.

  5. fejta-bot commented on Dec 3, 2020

    @fejta-bot

    Issues go stale after 90d of inactivity.
    Mark the issue as fresh with /remove-lifecycle stale.
    Stale issues rot after an additional 30d of inactivity and eventually close.

    If this issue is safe to close now please do so with /close.

    Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
    /lifecycle stale

  6. added
    lifecycle/staleDenotes an issue or PR has remained open with no activity and has become stale.
    on Dec 3, 2020
  7. fejta-bot commented on Jan 2, 2021

    @fejta-bot

    Stale issues rot after 30d of inactivity.
    Mark the issue as fresh with /remove-lifecycle rotten.
    Rotten issues close after an additional 30d of inactivity.

    If this issue is safe to close now please do so with /close.

    Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
    /lifecycle rotten

  8. added
    lifecycle/rottenDenotes an issue or PR that has aged beyond stale and will be auto-closed.
    and removed
    lifecycle/staleDenotes an issue or PR has remained open with no activity and has become stale.
    on Jan 2, 2021
  9. brendandburns commented on Jan 9, 2021

    @brendandburns
    ContributorAuthor

    /lifecycle frozen

  10. added
    lifecycle/frozenIndicates that an issue or PR should not be auto-closed due to staleness.
    and removed
    lifecycle/rottenDenotes an issue or PR that has aged beyond stale and will be auto-closed.
    on Jan 9, 2021
  11. added
    good first issueDenotes an issue ready for a new contributor, according to the "help wanted" guidelines.
    on Jan 9, 2021
  12. Priya103 commented on Jan 20, 2021

    @Priya103

    If still available I would like to contribute

  13. 1 remaining item

  14. Vyom-Yadav commented on Aug 18, 2022

    @Vyom-Yadav

    @brendandburns I have one doubt, I was working on util/src/test/java/io/kubernetes/client/informer/cache/SharedProcessorTest.java, particularly on:

    @Test
    public void testListenerAddition() throws InterruptedException {
    SharedProcessor<V1Pod> sharedProcessor = new SharedProcessor<>();
    V1Pod foo1 = new V1Pod().metadata(new V1ObjectMeta().name("foo1").namespace("default"));
    ProcessorListener.Notification<V1Pod> addNotification =
    new ProcessorListener.AddNotification<V1Pod>(foo1);
    ProcessorListener.Notification<V1Pod> updateNotification =
    new ProcessorListener.UpdateNotification<V1Pod>(null, foo1);
    ProcessorListener.Notification<V1Pod> deleteNotification =
    new ProcessorListener.DeleteNotification<V1Pod>(foo1);
    ExpectingNoticationHandler<V1Pod> expectAddHandler =
    new ExpectingNoticationHandler<V1Pod>(addNotification);
    ExpectingNoticationHandler<V1Pod> expectUpdateHandler =
    new ExpectingNoticationHandler<V1Pod>(updateNotification);
    ExpectingNoticationHandler<V1Pod> expectDeleteHandler =
    new ExpectingNoticationHandler<V1Pod>(deleteNotification);
    sharedProcessor.addAndStartListener(expectAddHandler);
    sharedProcessor.addAndStartListener(expectUpdateHandler);
    sharedProcessor.addAndStartListener(expectDeleteHandler);
    sharedProcessor.distribute(addNotification, false);
    sharedProcessor.distribute(updateNotification, false);
    sharedProcessor.distribute(deleteNotification, false);
    // sleep 1s for notification distribution completing
    Thread.sleep(1000);
    assertTrue(expectAddHandler.isSatisfied());
    assertTrue(expectUpdateHandler.isSatisfied());
    assertTrue(expectDeleteHandler.isSatisfied());
    }

    I can create a semaphore and acquire it, blocking the main thread, but how will I release so that blocked main thread will continue the execution, I looked at many tests, facing similar problem in many of them.

    Edit-
    I cannot access ExecutorService instance variable which would have been useful over here, please suggest.

  15. vinayak912002 commented on Aug 22, 2022

    @vinayak912002

    what are the things I need to learn in order to start contributiing to this project?

  16. Vyom-Yadav commented on Aug 27, 2022

    @Vyom-Yadav
  17. tmrpavan commented on Apr 10, 2023

    @tmrpavan

    /assign

  18. abhinavvrm2020 commented on Oct 12, 2025

    @abhinavvrm2020

    @brendandburns Is this issue still open. I would like to fix these tests.

  19. priyanshuvishwakarma273403 commented on Jun 20, 2026

    @priyanshuvishwakarma273403

    Hi @maintainer,
    I would like to work on this issue. Could you please assign it to me?

  20. flrnd commented on Sep 28, 2026

    @flrnd

    Hey,

    I see there are already PRs open for:

    1. Remove Thread.sleep() from DefaultWorkQueueTest #4910
    2. Remove Thread.sleep() from port forward test class #4911
    3. Fix: removing Thread.sleep() from KubernetesReconcilerCreatorTest #4915

    To avoid duplicating effort, I'm going to start working on the remaining items from the bottom of the list, beginning with extended/src/test/java/io/kubernetes/client/extended/event/EventCorrelatorTest.java.

    /assign

  21. flrnd commented on Oct 5, 2026

    @flrnd

    Less time available than I anticipated, so I’m unassigning myself. I’m still planning to work on some of the tests, just without taking ownership of the issue.

    /unassign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

good first issueDenotes an issue ready for a new contributor, according to the "help wanted" guidelines.lifecycle/frozenIndicates that an issue or PR should not be auto-closed due to staleness.

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions