23

Let's imagine I have the following method which should be tested:

@Autowired
private RoutingService routingservice;

public void methodToBeTested() {
    Object objectToRoute = initializeObjectToRoute();
    if (someConditions) {
         routingService.routeInOneWay(objectToRoute);
    } else {
         routingService.routeInAnotherWay(objectToRoute);
    }
}

In this case RoutingService is running in the separate thread, thus in it's constructor we have the following:

Thread thread = new Thread(this);
thread.setDaemon(true);
thread.start();

The problem is that RoutingService changes the state of objectToRoute and this is exactly what I want to check, but this doesn't happen straight away thus the test fails. However, if I add Thread.sleep() then it works, but this is bad practice as I know.

How can I avoid Thread.sleep() in this case?

Rufi
  • 2,529
  • 1
  • 20
  • 41
  • 1
    Using `Thread.sleep` in a unit test isn't bad practice, as all you are doing is mimicking the passage of time, which is necessary for some unit tests. The reason people say that using `Thread.sleep` is a bad practice is because it is sometimes used as an attempt to fix race condition. Are you only using the `Thread.sleep` in the tests, or in your source code? – Ben Green Mar 29 '16 at 11:56
  • When you are testing the threaded code, how would you ensure your unit tests are not flaky? That they always execute in a deterministic fashion. I think mocking the Service class is a better opiton – Anupam Saini Mar 29 '16 at 12:00
  • I am using sleep only in tests, but found quite a few places where people mention that generally sleep is not that good in unit tests. For example, also SonarLint plugin is complaining and saying that it is violation and give the following description: "Using Thread.sleep in a test is just generally a bad idea. It creates brittle tests that can fail unpredictably depending on environment ("Passes on my machine!") or load." – Rufi Mar 29 '16 at 12:01
  • Agree with Ben. We run JUnit tests during our Maven builds and the current thread doesn't wait for multithreaded processes, like JMS interactions and external performance monitoring. Thread.sleep is what we use to ensure those processes complete before the thread is killed. – MolonLabe Mar 29 '16 at 12:05
  • Won't you agree that Sleep makes unit tests slower at least? Shouldn't I try to avoid it? – Rufi Mar 29 '16 at 12:11
  • perhaps it helps: [Testing Thread.start without sleep](http://stackoverflow.com/a/39525700/3016686) – Ultimate Fighter Sep 16 '16 at 08:23
  • See also https://stackoverflow.com/questions/12159/how-should-i-unit-test-threaded-code – Raedwald Nov 12 '17 at 10:57

7 Answers7

8

If you are testing [Unit Test] for the method methodToBeTested, you should simply mock routingservice.
You shouldn't be testing any methods that methodToBeTested calls.

However, it sounds like you want to test the RoutingService (you said "The problem is that RoutingService changes the state of objectToRoute and this is exactly what I want to check").

To test RoutingService methods, you should write separate unit tests for those methods.

Ahmed Nabil
  • 17,392
  • 11
  • 61
  • 88
forgivenson
  • 4,394
  • 2
  • 19
  • 28
  • Yes, this might be one way, however, i cannot say that I like it. Most probably my test moves from Unit test to Integration test then. RoutingService is tested, but in this case I want to verify that under certain conditions the routing was done, however, it might take some time. Of course, I can mock RoutingService and verify that specific method was called, but then I will not cover the state change. – Rufi Mar 29 '16 at 12:07
  • Yes, I would say if you want to do this, it becomes an integration test, and shouldn't be run with the normal set of unit tests. unit tests should complete quickly and consistently, so developers can run them all before checking in any code. Have a separate set of integration tests that get run on a regular basis (such as via Jenkins) – forgivenson Mar 29 '16 at 12:18
7

I suggest awaitility for synchronizing asynchronous tests. For example, assume you have an result object that get set after some thread operations and you want to test it. You write statement like this:

 await()
.atMost(100, TimeUnit.SECONDS)
.untilAsserted(() -> assertNotNull(resultObject.getResult()));

It waits maximum 100 seconds, or until the assertion is satisfied. For instance, if getResult() returns something that is not null in between 0-100 seconds, the execution continues unlike Thread.sleep which holds the execution for given time no matter result is there or not.

sarah
  • 580
  • 1
  • 6
  • 24
  • 5
    just wondering what's the profit here? unless we can only win some extra time so tests are a bit faster... but still it's similar to thread.sleep just with some syntax-sugar – java_newbie Mar 26 '19 at 13:41
  • 1
    @java_newbie: From SonarLint Rule Description(java:S2925):`Using Thread.sleep in a test is just generally a bad idea. It creates brittle tests that can fail unpredictably depending on environment ("Passes on my machine!") or load. Don’t rely on timing (use mocks) or use libraries such as Awaitility for asynchroneous testing.` – jumping_monkey May 31 '22 at 04:53
2

You could mock objectToRoute to set the value of a CompletableFuture and then call get on that in your assertion. This will wait until the value is set before continuing. Then set a timeout @Test(timeout=5000) in case the value is never set.

This has the advantage that the test won't wait longer than necessary and it's harder to fail because of too short a time because you can make the timeout much larger than normal.

fgb
  • 18,439
  • 2
  • 38
  • 52
0

It depends. As Ben Green said in his comment, sleep is dangerous in tests because it can hide a race condition. You know if the overall design contains such a race condition, where the service could be used before the route is ready. It it does, you should fix it in code, for example by testing a ready condition, and test it the same in your test class.

If you know that it cannot happen, you should document it in you main code and in your test class. That would then be a perfect justification for a sleep.

(I assume that this is for an integration test - for unit tests mocks should be enough as you were said in other answers)

Serge Ballesta
  • 143,923
  • 11
  • 122
  • 252
0

Use Object.wait()

We have worked with some asynchronous processes that obtain files from directories or .ZIP archives. We use the content of the files somewhere else while the asynchronous one keeps on reading.

The way we came around to test them was to wait() on the communication object --in our case it was a Queue--, so the asynchronous process would queue.notifyAll() whenever there was a new file ready to be used and kept working. On the other end the consumer would process one item at a time until the queue was empty and then queue.wait() to wait for more. We do use a special object passed through the queue to indicate that there are no more items to process.

In your case I suppose the object you want to check, objectToRoute, is not really created inside the method you want to test. You could wait() on it inside the test, and notifyAll() on it inside the method you want to test, methodToBeTested. I know this would introduce the extra line of code in your production code base, but with nobody waiting on it, it should result harmless. It would end up something like:

public void methodToBeTested(Object objectToRoute) {
    if (someConditions) {
         routingService.routeInOneWay(objectToRoute);
    } else {
         routingService.routeInAnotherWay(objectToRoute);
    }
    synchronized(objectToRoute) {
        objectToRoute.notifyAll();
    }
}

And in your test class there will be something like:

@Test
public void testMethodToBeTested() throws InterruptedException {
    Object objectToRoute = initializeObjectToRoute();
    methodToBeTested(objectToRoute);
    synchronized (objectToRoute) {
        objectToRoute.wait();
    }
    verifyConditionsAfterRouting(objectToRoute);
}

I do know that in this simple example does not make too much sense, since the sample system is not multithreaded. I assume the multithread twist is added in the routeInOneWay and routeInAnotherWay methods; thus, those are the ones to call notifyAll() method.

As sample code to point the direction of our solution, here are some code fragments.

In the asynchronous worker side, or producer:

while(files.hasNext(){
   queue.add(files.next());
   synchronized (outputQueue) {
       queue.notifyAll()
   }
}

And in the consumer side:

while(!finished){
    while(!queue.isEmpty()){
        nextFile = queue.poll();
        if (nextFile.equals(NO_MORE_FILES_SIGNAL)) {
            finished = true;
            break;
        }
        doYourThingWith(nextFile);
    }
    if (!finished) {
        synchronized (outputQueue) {
            outputQueue.wait();
        }
    }
}
manuelvigarcia
  • 1,696
  • 1
  • 22
  • 32
0

Since java 9 you can use the delayedExecutor method of CompleteableFuture to avoid Thread.sleep and its SonarLint warnings.

This is the basic pattern, adjust to your needs:

import static java.util.concurrent.CompletableFuture.delayedExecutor;
import static java.util.concurrent.CompletableFuture.runAsync;
import static java.util.concurrent.TimeUnit.MILLISECONDS;

@Test
void yourTest() {

// [...]

  runAsync(() -> {}, delayedExecutor(100, MILLISECONDS)).join();
}
Andy Brown
  • 11,766
  • 2
  • 42
  • 61
-3

Instead of avoiding Thread.sleep(), You can pass value as zero in Junit Test Case.