0

My company is finally starting to introduce unit tests. While going through some of the new tests, I came across this one:

[Test]
public void GetCorrectTimeoutInfoFromTimeSpan()
{
    // Arrange
    bool preTimeout;
    bool postTimeout;
    double random = new Random().NextDouble();
    TimeoutHandler timeoutHandler;

    // Act
    timeoutHandler = new TimeoutHandler(TimeSpan.FromMilliseconds(1));
    preTimeout = timeoutHandler.IsTimeout;
    while(!timeoutHandler.IsTimeout)
    {
        // Simulate some work
        random = Math.Sin(random) * Math.Cos(random);
    }
    GC.KeepAlive(random);
    postTimeout = timeoutHandler.IsTimeout;

    // Assert
    Assert.Multiple(() =>
    {
        Assert.That(preTimeout, Is.False);
        Assert.That(postTimeout, Is.True);
    });
}

The variable random is only there to simulate some work, ensured to not be "optimized" away via the GC.KeepAlive(). Since this is will be reused in many different situations where we need to simulate work, I wanted to see if there is a better/cleaner way.

Off the top of my head I would think something like this might work

{...}
double? rnd = null;
while(!timeoutHandler.IsTimeout)
    rnd = SimulateWork(rnd);
{...}

public static double SimulateWork(double? rnd = null)
{
    rnd ??= new Random().NextDouble();
    return Math.Sin(rnd.Value) * Math.Cos(rnd.Value);
}

This would seperate the dependency for random values and as far as I know C# should keep this code as is, making the specific call to garbage collector not needed.

As it still requires a local variable, I am not sure if this is truly better or I am just overthinking things.

Improve this question
6
  • 2
    I can't really see the point of trying to keep the CPU busy rather than just doing Thread.Sleep(). What is the purpose of that? Commented Aug 12 at 10:27
  • new Random() all the time also kind of raises a red flag. That's really not a cheap thing to do. The more UnitTests you'll have, the more this will become significant to overall tests execution time. Commented Aug 12 at 11:32
  • 2
    I'm pretty sure that GC.KeepAlive(random) has no effect and will just confuse future readers. Commented Aug 12 at 11:42
  • 1
    Technically this is not a unit test. It's an integration test, because it depends on the system clock. TimeoutHandler should inject ITimeProvider. In a unit test you would then give it a FakeTimeProvider and manipulate the time by code instead of "simulating work". Commented Aug 12 at 11:44
  • 1
    Sorry but this test is (a) useless and (b) bonkers. It's asserting that postTimeout is true when it can't possibly be anything else, the while loop wouldn't exit if it were false. Also you don't need to simulate work and loop, just wait for however long before you expect to time out, and really, really, never, ever put random in a test! Tests are supposed to be reproduceable. Commented Aug 12 at 15:12

2 Answers 2

Reset to default
1

This will depend on how accurate simulation of a load you need.

To simulate an actual CPU load I would suggest using Thread.SpinWait(int iteratons) in a loop;

var sw = Stopwatch.StartNew();
while(sw.ElapsedMilliseconds < 1){
    Thread.SpinWait(10000);
}

I'm fairly sure the compiler is not allowed to optimize away the spinwait, since that would defeat the purpose. This could be useful if you want to test behavior under high system load, to help find race conditions or other threading issues that might not be reproducible in low load scenarios.

Other options are Thread.Sleep(int milliseconds) or await Task.Delay(int milliseconds). These would be the typical approach if you just need to wait some time, and are not testing threading issues specifically.

But my recommendation is to design your classes so they do not depend on timing, since that help avoiding spuriously fails, and ensure they are fast to execute. The general approach would be to abstract away anything that depend on time:

  • If you are buffering work to run in the background, add a Flush() method that blocks until the buffer is cleared, or do the same thing in the dispose method.
  • If you are using timers, inject a custom timer interface. This should allow you to inject a mock implementation that "ticks" when the test commands it to.
  • If you use DateTime.Now, try taking it as parameter input if possible, or inject an interface and provide a mock implementation.

Naturally, this would not work if you need to test your actual timer implementation. But the goal is to make this so simple that there is little that could go wrong. Unit tests tend to be most valuable when testing complicated logic that is easy to get wrong, testing a simple wrapper around a framework provided class provides little value.

In your example it is possible that your thread gets preempted just after TimeoutHandler is created, and that preTimeout is true. Such spurious fails significantly reduces the value of unit testing, since you do not know if a fail is an actual bug that is just difficult to reproduce, or just a expected effect of running on a no real time OS.

Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

1

Simply use Thread.Sleep as suggested, or use asynchronous equivalent (more modern approach): Task.Delay.

So simply do

await Task.Delay(2); // or as many milliseconds as needed
Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.