5

I have a repeated pattern in my code that is not easy to test: classes that execute periodically. Just to simplify things lets say assume something like:

while( running ){

    long millisToWait = scheduler.getMillisecondsUntilNextExecution();
    Thread.sleep(millisToWait);
    executeMyTask();
}

Where Thread.sleep is an static method that is hard to change. It doesn't matter if I inject a custom Clock into the scheduler because changing the clock will not change the sleep time. I think that the solution may be to create a ThreadSleeper Class.

System.currentMillis        -> replace by injected Clock.millis();
Thread.sleep(millisToWait)  -> replace by injected ThreadSleeper.sleep(millisToWait)

Where ThreadSleeper may be something like;

import java.util.concurrent.Semaphore;

public abstract class ThreadSleeper {

    public abstract void sleep(long millisecondsToSleep) throws InterruptedException;

    public static SystemThreadSleeper systemThreadSleeper(){
        return new SystemThreadSleeper();
    }

    public static ThreadSleeper testThreadSleeper(){
        return new TestThreadSleeper();
    }

    public static final class TestThreadSleeper extends ThreadSleeper{

        private Semaphore semaphore = new Semaphore(0);
        private Long millisecondsToSleep = null;

        //FIXME: make usable my multithread.
        //TODO: add method simulateMillisecondsPasses.

        @Override
        public synchronized void sleep(long millisecondsToSleep) throws InterruptedException {
            this.millisecondsToSleep = millisecondsToSleep;
            semaphore.acquire();
        }

        public synchronized long simulateSleepEndsReturnTimeSlept(){
            if(semaphore.availablePermits() < 0){
                semaphore.release();
                long slept = millisecondsToSleep;
                millisecondsToSleep = null;
                return slept;
            }else{
                return 0L;
            }
        }
    }

    public static final class SystemThreadSleeper extends ThreadSleeper{

        public void sleep(long millisecondsToSleep) throws InterruptedException {
            Thread.sleep(millisecondsToSleep);
        }
    }
}

Is this a reasible/good practice? I wonder why I have never seen something like this. May be it is an overkill or maybe there are better alternatives.

  • 1
    Why can't you just have scheduler.getMillisecondsUntilNextExecution() return a smaller number for testing? – nvoigt Jul 24 '18 at 15:52
  • @nvoigt For some test this works. But I want to check that the process is executed after certain periods. – Borjab Jul 24 '18 at 15:57
  • I'm afraid I don't get the difference... lets say you want to make sure it works, you let the scheduler return 1s, run it for 4s and see that it executed at least 3 times. Why do you need this to be a larger number? – nvoigt Jul 24 '18 at 16:00
  • 1
    its not good practice because sleeping the thread for long periods to delay execution is not good practice – Ewan Jul 24 '18 at 16:03
  • 2
    use a Timer. read the interval from config or injection. or better yet have a ShouldRun function you call every small interval – Ewan Jul 24 '18 at 16:25
4

Is this a reasonable/good practice?

Absolutely.

Justification: time is an input.

If you don't consider time an input value, think about it until you do - it is an important concept

John Carmack, 1998

It's a normal thing in our isolated tests, to provide strategies for managing side effects. The passage of time is one side effect where - in a test - we might want an alternative strategy.

I would normally reach first for an interface, rather than an abstract class

interface Sleeper {
    void sleep(long durationInMillis) throws InterruptedException;
}

You are absolutely right that this strategy has a corresponding read that goes along with it -- one of the things that we expect when we sleep is that afterwards the measurements from the clock have changed. The implementations of these strategies are implicitly coupled -- but the caller doesn't need to care.

More generally: in your production code, you are making a decision that sleep should be implemented using java threads. Parnas teaches us that hiding the details of these decisions behind a module boundary is a good idea.

3

I will say this over and over:

Relying on timing causes slow and fragile tests

If you need to ensure a task has completed, then you need some other synchronization mechanism than simply relying on the fact enough time has gone by. If you are using C# then you can simply await the task to complete. However, there are other options like thread Barriers which allows a background task to signal it's completion to the foreground task.

You would use the barrier something like this pseudocode:

// Size is number of participants, one for the test and one for the task.
var barrier = new Barrier(2);

executeMyTask(barrier);
barrier.Await(); // can use variant with max ms to wait

Then in your task, the last statement would be:

barrier.Await();

Since the test would already be blocked, this will essentially unblock the test and this task.

So what if my test is about timing how long the task takes?

You essentially have a stop watch (C# provides a class for this purpose, or you can take timestamps before and after). Perform your test like this:

  • Start the stopwatch
  • Start the task
  • Await your task or block with the Barrier
  • Stop the stopwatch
  • Assert that the time taken is within an acceptable margin of error.

Most of the time you just need to make sure the task was less than a certain amount of time, but if it takes a few milliseconds more it might not be a problem. Sometimes you need to ensure it actually took X number of milliseconds--but there is still an acceptable margin of error.

  • 1
    The sleep isn't in the test, it's in the code being tested. – Karl Bielefeldt Jul 24 '18 at 22:23
  • It still begs the question, what problem is that sleep supposed to solve? In most cases any call to sleep is compensating for something else. I've been bitten many times by thinking that sleep was good enough, and it's a fragile fix. – Berin Loritsch Jul 25 '18 at 2:20
1

It seems like the executeMyTask method is the thing you want to test.

The other bit is just a simple while loop that doesn't need to be unit tested. Instead of injecting a "thread sleeper" I would inject a "task executor" or "task" that gets executed:

while( running ){
    long millisToWait = scheduler.getMillisecondsUntilNextExecution();
    Thread.sleep(millisToWait);

    // Injected via constructor injection
    task.execute();
}

Now you need an interface and a concrete implementation:

public interface ScheduledTask {
    void execute();
}

public class MyScheduledTask {
    public void execute() {
        // The thing you want to do
    }
}

Then you can unit test the MyScheduledTask class since it's execute method is public.

0

There is no single true answer, but you could unite sleeping and getting current time into one interface. Because they are both related to time, aren't they? Then for mocking you could use simple incrementing value.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.