Skip to content

.Net: [.NET] Add TimeProvider injection to TimePlugin for deterministic testing#14112

Open
grvnmttl wants to merge 2 commits into
microsoft:mainfrom
grvnmttl:timeplugin-clock-injection
Open

.Net: [.NET] Add TimeProvider injection to TimePlugin for deterministic testing#14112
grvnmttl wants to merge 2 commits into
microsoft:mainfrom
grvnmttl:timeplugin-clock-injection

Conversation

@grvnmttl

Copy link
Copy Markdown

Closes #14111

Summary

  • Adds a TimeProvider constructor parameter to TimePlugin (defaults to TimeProvider.System, so all existing callers are unaffected)
  • Replaces all direct DateTimeOffset.Now / DateTimeOffset.UtcNow calls with _timeProvider.GetLocalNow() / _timeProvider.GetUtcNow()
  • Rewrites TimePluginTests from 5 real-clock tests to 26 deterministic tests covering every public method, using an inline FixedUtcTimeProvider subclass (no new test dependencies)

Key decisions

Decision Reason
System.TimeProvider (not a custom IClock) Standard .NET 8+ pattern; no extra abstraction needed
Inline FixedUtcTimeProvider subclass in tests Avoids adding Microsoft.Extensions.TimeProvider.Testing as a new dependency; 3-line subclass is clearer
LocalTimeZone = TimeZoneInfo.Utc on the test provider Makes test output identical on any machine regardless of OS timezone
Default TimeProvider.System in constructor Zero breaking change — all existing callers (new TimePlugin()) work unchanged

Test results

Passed! - Failed: 0, Passed: 394, Skipped: 0, Total: 394

… tests

Replaces direct DateTimeOffset.Now calls with an injected TimeProvider,
defaulting to TimeProvider.System so existing callers are unaffected.
Expands TimePluginTests from 5 fuzzy real-clock tests to 26 deterministic
tests covering all public methods.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@grvnmttl grvnmttl requested a review from a team as a code owner June 23, 2026 04:13
Copilot AI review requested due to automatic review settings June 23, 2026 04:13
@moonbox3 moonbox3 added the .NET Issue or Pull requests regarding .NET code label Jun 23, 2026
@github-actions github-actions Bot changed the title [.NET] Add TimeProvider injection to TimePlugin for deterministic testing .Net: [.NET] Add TimeProvider injection to TimePlugin for deterministic testing Jun 23, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes TimePlugin deterministic-test friendly by introducing TimeProvider injection and updating all “current time” computations to use the injected provider rather than directly reading the system clock. It also rewrites the unit tests to use a fixed-time provider and validate formatted outputs deterministically.

Changes:

  • Add TimeProvider support to TimePlugin and route time reads through it (GetLocalNow() / GetUtcNow()).
  • Replace real-clock dependent tests with deterministic tests using an inline fixed TimeProvider implementation.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
dotnet/src/Plugins/Plugins.Core/TimePlugin.cs Introduces TimeProvider-based clock access and updates time/date formatting methods accordingly.
dotnet/src/Plugins/Plugins.UnitTests/Core/TimePluginTests.cs Replaces non-deterministic tests with fixed-time, culture-stable assertions across plugin methods.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread dotnet/src/Plugins/Plugins.Core/TimePlugin.cs
Comment thread dotnet/src/Plugins/Plugins.Core/TimePlugin.cs
Comment thread dotnet/src/Plugins/Plugins.Core/TimePlugin.cs
Comment thread dotnet/src/Plugins/Plugins.UnitTests/Core/TimePluginTests.cs
@grvnmttl

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

- Fix TimeZoneName() to use _timeProvider.LocalTimeZone instead of
  TimeZoneInfo.Local, so a custom provider's timezone is respected
- Add deterministic TimeZoneName test (395 tests total, all passing)
- Add Microsoft.Bcl.TimeProvider polyfill for netstandard2.0 target

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@grvnmttl

Copy link
Copy Markdown
Author

All four Copilot review items addressed in the latest commit:

  1. Binary breaking change — kept the single optional-parameter constructor (TimeProvider? timeProvider = null). Adding a separate explicit parameterless overload would cause ActivatorUtilities (used by KernelPluginFactory.CreateFromType) to throw on ambiguity. Since Plugins.Core is a preview package with no stable release, there are no compiled consumer binaries to break; all existing new TimePlugin() call sites remain source-compatible.
  2. netstandard2.0 compatibility — added Microsoft.Bcl.TimeProvider (8.0.1) to Directory.Packages.props and as a netstandard2.0-only PackageReference in Plugins.Core.csproj.
  3. TimeZoneName() uses injected timezone — now returns _timeProvider.LocalTimeZone.DisplayName instead of TimeZoneInfo.Local.DisplayName.
  4. TimeZoneName() test — added; asserts the provider's LocalTimeZone is reflected. Test count: 395, all passing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

.NET Issue or Pull requests regarding .NET code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

.Net: [.NET] TimePlugin: add TimeProvider injection for testability

3 participants