Timeprovider advance multiple invocations#4022
Conversation
6e06690 to
d1abbb4
Compare
|
I didn't want to change too much of the existing implementation, but an alternative approach to implementing this I've used in my ManualTimeProvider is to only hold waiters in the |
src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProvider.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProvider.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProvider.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProvider.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProvider.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProviderTimer.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProviderTimer.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProvider.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProvider.cs
Show resolved
Hide resolved
debbca7 to
1ee526e
Compare
|
I had a brief chat with @sebastienros, and it looks like I may have misinterpreted his comments. I am removing myself from this PR and letting @sebastienros and @geeknoid take over the reviews. |
| } | ||
|
|
||
| private List<FakeTimeProviderTimer.Waiter> GetWaitersToWake() | ||
| private FakeTimeProviderTimer.Waiter? TryGetWaiterToWake(DateTimeOffset targetNow) |
There was a problem hiding this comment.
Could it use the standard Try... pattern by being bool TryGetWaiterToWake(DateTimeOffset targetNow, out FakeTimeProviderTimer.Waiter?)
There was a problem hiding this comment.
Yes. But please indulge me, why?
I would use the common bool TryGetX(Foo foo, out Foo bar) in a public API surface since that is a very recognizable pattern in the .NET ecosystem. For private/internal stuff, I have changed to this pattern since we now have nullable reference types and pattern matching, I feel this version is more direct, clean, and doesn't involve out arguments. I am fairly certain that had these language features been around in the early days, the TryGetX pattern would not be the norm.
I do respect that you may have a coding standard in this repo you like to follow (I admit I didn't look at other projects in the repo, just this one) and I would change it regardless if you prefer the other style.
|
@sebastienros / @geeknoid I've added you to my fork of the repo so you can push changes to this PR as you see fit. |
src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProvider.cs
Outdated
Show resolved
Hide resolved
It is often useful to have the epoch/start time available via the FakeTimeProvider during testing, as it provides a stable offset (time-0) from which to advance or set a new utc-now based off, e.g. to move to time-X on a timeline starting at time-0.
|
Let me add the missing experimental attribute to the ctor and push a rewrite of the git history that bundles up the feedback into clean commits. Ps. I do not have the permissions to merge the PR. |
e8010ee to
8e3c86e
Compare
…ance fixes dotnet#3995 Reorder variable's in comparaison Co-authored-by: Igor Velikorossov <RussKie@users.noreply.github.com> remove unneeded comments Squashed commit of the following: commit 2f51f4063e2c0ec9042d425b50380f8c81ab6838 Author: Egil Hansen <egil@assimilated.dk> Date: Tue May 30 16:37:13 2023 +0000 All changes
8e3c86e to
145515a
Compare
This fixes #3995. These are the changes I have included:
FakeTimeProviderTimer.Waiternow tracks when aWakeupTimewas scheduled. It does this to ensure that the callbacks are invoked in the order they are scheduled in.FakeTimeProviderTimernow triggers callbacks one at a time, and allows their callbacks to add reschedule themselves or influence other timers/create new timers.FakeTimeProviderexposes anEpochproperty, which represents the initial start time when the time provider was instantiated. It is a useful value to have available during testing, e.g. if you have to callSetUtcNowmultiple times during a test, and the value you are setting is changed in relation to the start time/epoch. It tends to lead to easier-to-understand test code in my experience.I also changed the ctor argument
startTimeonFakeTimeProvidertoepochto match the name of the added property.Advance()andSetUtcNow()to prevent moving time back.Microsoft Reviewers: Open in CodeFlow