Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FakeTimeProvider does not advance when GetTimestamp is called #5694

Open
ItsVeryWindy opened this issue Nov 25, 2024 · 4 comments
Open

FakeTimeProvider does not advance when GetTimestamp is called #5694

ItsVeryWindy opened this issue Nov 25, 2024 · 4 comments
Labels
area-fundamentals bug This issue describes a behavior which is not expected - a bug.

Comments

@ItsVeryWindy
Copy link

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

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

Maybe it needs to do something similar to

@ItsVeryWindy ItsVeryWindy added bug This issue describes a behavior which is not expected - a bug. untriaged labels Nov 25, 2024
@RussKie
Copy link
Member

RussKie commented Nov 25, 2024

/cc: @dotnet/dotnet-extensions-fundamentals

@amadeuszl
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

amadeuszl commented Nov 29, 2024

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.

@amadeuszl amadeuszl reopened this Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-fundamentals bug This issue describes a behavior which is not expected - a bug.
Projects
None yet
Development

No branches or pull requests

3 participants