dotnet / extensions

This repository contains a suite of libraries that provide facilities commonly needed when creating production-ready applications.
MIT License
2.69k stars 759 forks source link

FakeTimeProvider does not advance when GetTimestamp is called #5694

Open ItsVeryWindy opened 1 week ago

ItsVeryWindy commented 1 week ago

Description

This is how I expect it to work, not sure if it's a bug, intentional or an oversight.

var advanceAmount = TimeSpan.FromSeconds(5);

var provider = new FakeTimeProvider
{
    AutoAdvanceAmount = TimeSpan.FromSeconds(5)
};

/// .. non test code

var startingTimestamp = provider.GetTimestamp();

/// ... things happening in between

var elapsedTime = provider.GetElapsedTime(startingTimestamp);

/// ... test code

// expected to be true but is 0
Assert.That(elapsedTime, Is.EqualTo(advanceAmount))

Reproduction Steps

As above ^^

Expected behavior

I would expect that when checking the elapsed time that it should match what is specified in AutoAdvanceAmount.

Actual behavior

The elapsed time is zero.

Regression?

No idea.

Known Workarounds

Currently I can work around the issue by advancing the timer myself in between the calls to GetTimestamp and GetElapsedTime.

Configuration

.NET 8

Other information

https://github.com/dotnet/extensions/blob/cfed375f3161f2e553e946b4f968b818e8e858f1/src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProvider.cs#L165

This shows that it just returns the number of ticks from now.

Maybe it needs to do something similar to https://github.com/dotnet/extensions/blob/cfed375f3161f2e553e946b4f968b818e8e858f1/src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProvider.cs#L83

RussKie commented 1 week ago

/cc: @dotnet/dotnet-extensions-fundamentals

amadeuszl commented 6 days ago

Thank you for raising this question. I did some checks and current implementation works as expected. There's one important reason why GetTimestamp() does not AutoAdvance. GetElapsedTime() runs GetTimestamp() as well in the implementation in TimeProvider, that would cause unintended move in time when just checking the time with GetElapsedTime(). Here's implementation for GetElapsedTime().

ItsVeryWindy commented 6 days ago

Personally I'd consider that an acceptable trade off, I'm not even sure I would call it a trade off...

If I did this, would I normally expect the same value twice?

var elapsedTime = provider.GetElapsedTime(startingTimestamp);
var elapsedTime2 = provider.GetElapsedTime(startingTimestamp);
amadeuszl commented 5 days ago

I see the point in your example. If we would implement it the same way as for GetUtcNow() like in below example the advance happens but returned value is the one before the advancement.

    public override long GetTimestamp()
    {
        DateTimeOffset result;
        lock (Waiters)
        {
            result = _now;
            _now += _autoAdvanceAmount;
        }

        WakeWaiters();
        return result.Ticks;
    }

I was also concerned about introducing minor breaking change, but impact of this change would not have influence on users that use GetElapsedTime() just once as the result from GetTimestamp() would be the same.

Another example to consider:

    [Fact]
    public void GetElapsedTime_WithAutoadvance()
    {
        var timeProvider = new FakeTimeProvider()
        {
            AutoAdvanceAmount = TimeSpan.FromSeconds(1)
        };

        var st = timeProvider.GetTimestamp();
        var elapsedTime = timeProvider.GetElapsedTime(st);
        var elapsedTime2 = timeProvider.GetElapsedTime(st);

        Assert.Equal(TimeSpan.FromSeconds(1), elapsedTime);
        Assert.Equal(TimeSpan.FromSeconds(2), elapsedTime2);
    }

I initially thought it would result in elapsedTime being 2 seconds not 1, but that's not the case. Given this example it seems like reasonable API and behavior underneath to have.

That being said this proposition would make sense. I'm reopening this to hear feedback also from other people.