fix: MockTimeProvider constructor treats "now" having DateTimeKind.Unspecified as if it had DateTimeKind.Local #834
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…fied as if it had DateTimeKind.Local The difference between "DateTime.Now" And "DateTime.UtcNow" should be the offset from the local time zone to UTC. Before this change, the MockTimeSystem would produce a difference double that size when initialized with a DateTime having Kind DateTimeKind.Unspecified. The MockTimeSystem delegates to a MockDateTime wrapped around a TimeProviderMock. The TimeProviderMock would yield a DateTime with Kind DateTimeKind.Unspecified to the MockDateTime, which would then apply ToLocalTime() for "Now" or ToUniversalTime for "UtcNow". When applied to a DateTime with Kind DateTimeKind.Unspecified, ToLocalTime assumes that the value is in UTC, but ToUniversalTime assumes that it is Local. This discrepancy results in the doubling of the expected difference. Additionally, the test OnDateTimeRead_Today_ShouldExecuteCallbackWithCorrectParameter was failing, but only when run on systems outside of UTC. On systems running inside UTC, there was no difference between Now and UtcNow, so the error was hidden. So, when the MockTimeSystem is initialized with a DateTime which has Kind DateTimeKind.Unspecified, it should pick a specific Kind to use internally. Either Utc or Local would work. Local was selected for ergonomics: When a user of the MockTimeSystem specifies a literal DateTime without a Kind, e.g. "new Datime(2025, 8, 1, 9, 0, 0)", it seems more likely that they are intending to express a local time rather than a time in UTC.
vbreuss
reviewed
Aug 11, 2025
Source/Testably.Abstractions.Testing/TimeSystem/TimeProviderMock.cs
Outdated
Show resolved
Hide resolved
…nspecified as if it had a Kind of Utc. It was decided that Utc would be a less fragile default than Local.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the MockTimeProvider where DateTimeKind.Unspecified values were causing incorrect time zone offset calculations. The issue resulted in double the expected difference between DateTime.Now and DateTime.UtcNow when initialized with unspecified DateTime values.
- Converts DateTimeKind.Unspecified to DateTimeKind.Utc in TimeProviderMock constructor
- Adds comprehensive test coverage for the time zone offset bug
- Updates existing tests to use proper DateTimeKind.Local values
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Source/Testably.Abstractions.Testing/TimeSystem/TimeProviderMock.cs | Adds logic to convert DateTimeKind.Unspecified to DateTimeKind.Utc in constructor |
| Source/Testably.Abstractions.Testing/TimeProvider.cs | Documents the behavior for DateTimeKind.Unspecified parameters |
| Tests/Testably.Abstractions.Testing.Tests/MockTimeSystemTests.cs | Adds parameterized test for time zone offset behavior and fixes ToString test |
| Tests/Testably.Abstractions.Testing.Tests/TimeProviderTests.cs | Adds test for DateTimeKind.Unspecified conversion behavior |
| Tests/Testably.Abstractions.Testing.Tests/TimeSystem/NotificationHandlerTests.cs | Fixes test by using DateTimeKind.Local instead of unspecified |
|
This is addressed in release v4.3.2. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The difference between
DateTime.NowAndDateTime.UtcNowshould be the offset from the local time zone to UTC. Before this change,MockTimeSystemwould produce a difference double that size when initialized with aDateTimehavingKindDateTimeKind.Unspecified.The
MockTimeSystemdelegates to aMockDateTimewrapped around aTimeProviderMock. TheTimeProviderMockwould yield aDateTimewithKindDateTimeKind.Unspecifiedto theMockDateTime, which would then applyToLocalTime()forNoworToUniversalTimeforUtcNow. When applied to aDateTimewithKindDateTimeKind.Unspecified,ToLocalTimeassumes that the value is in UTC, butToUniversalTimeassumes that it is Local. This discrepancy results in the doubling of the expected difference.Additionally, the test
OnDateTimeRead_Today_ShouldExecuteCallbackWithCorrectParameterwas failing, but only when run on systems outside of UTC. On systems running inside UTC, there was no difference betweenNowandUtcNow, so the error was hidden.So, when the
MockTimeSystemis initialized with aDateTimewhich hasKindDateTimeKind.Unspecified, it should pick a specific kind to use internally. EitherUtcorLocalwould work.Utcwas selected because local times often lead to tests that only work in a specific time zone and should be selected intentionally.