-
Couldn't load subscription status.
- Fork 47
Refactor to use TimeProvider via injected service #785
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
base: main
Are you sure you want to change the base?
Conversation
- Replaced direct calls to TimeProvider.System.GetUtcNow() with injected TimeProvider service for better testability. - Added TimeProviderExtensions NuGet package. - Updated tests to utilize TimeProvider for timestamp control. - Introduced AddTimeProvider method in SharedDependencyConfiguration for service registration.
…atformServices] Instead of having a hardcoded string for platform specific keyed services, a custom attribute FromPlatformServicesAttributes is added and used instead. It makes code more readable and less error prone to typing mistakes.
There was a problem hiding this comment.
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 refactors the codebase to use TimeProvider via dependency injection instead of directly using TimeProvider.System.GetUtcNow(). The goal is to enable controlled time during testing while distinguishing between business logic that should use injectable TimeProvider and infrastructure code that should use DateTimeOffset.UtcNow.
- Creates a custom
FromPlatformServicesAttributefor easier keyed service injection - Registers TimeProvider as a keyed singleton service in dependency injection
- Replaces
TimeProvider.System.GetUtcNow()calls with injected TimeProvider in business logic - Uses
DateTimeOffset.UtcNowfor infrastructure code that cannot work with fake TimeProvider
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| SharedKernel.csproj | Added TimeProviderExtensions package dependency |
| AudibleEntity.cs | Changed to use DateTimeOffset.UtcNow for entity timestamps |
| SharedDependencyConfiguration.cs | Added TimeProvider registration as keyed service |
| FromPlatformServicesAttribute.cs | New custom attribute for platform service injection |
| Token generation files | Updated to use DateTimeOffset.UtcNow for JWT timestamps |
| Test files | Updated to use injected TimeProvider for controllable time |
| Repository/Handler files | Injected TimeProvider for business logic operations |
| EmailConfirmation domain | Updated methods to accept TimeProvider parameter |
| Directory.Packages.props | Added TimeProviderExtensions package version |
| AuthenticationCookieMiddleware.cs | Injected TimeProvider for token validation |
| backend.mdc | Updated coding guidelines for TimeProvider usage |
application/shared-kernel/SharedKernel/Configuration/SharedDependencyConfiguration.cs
Outdated
Show resolved
Hide resolved
application/shared-kernel/SharedKernel/Configuration/SharedDependencyConfiguration.cs
Outdated
Show resolved
Hide resolved
application/account-management/Tests/Signups/StartSignupTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Egil Hansen <egil@assimilated.dk>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Egil Hansen <egil@assimilated.dk>
Signed-off-by: Egil Hansen <egil@assimilated.dk>
|
Hi @egil, Thanks for the pull-request. I introduced You should run As you noticed, there are conventions for commit messages, which made the Pull Request Conventions fail. One line in imperative form (no description, as nobody sees them). Also, please squash your commits into two idiomatic commits (that is, remove "Fix spelling mistake", "Remove unused parameter", "Revert line breaks") ;) |
Summary & Motivation
While reading the documentation, I noticed "Use
TimeProvider.System.GetUtcNow()and notDateTime.UtcNow().".This does not make much sense, as using
TimeProvider.System.GetUtcNow()is the same as usingDateTime.UtcNow(). To leverageTimeProvider, you have to use the one available in the DI container. That, when done, allows you to use a fakeTimeProviderduring testing, which allows you to control time as well as timers.In my experience, what you want to do is have a keyed singleton service for a
TimeProviderthat all your application logic uses. That way, when you need to control time during testing, you are only affecting time for code you own, and know how behave related to time. This leads to more stable tests.Thus, I have refactored the use of
TimeProvider.System.GetUtcNow()to using a shared TimeProvider.There are also a few places where
TimeProvider.System.GetUtcNow()has been replaced byDateTimeOffset.UtcNow(), because time cannot be controlled for them anyway - they work with 3rd party that usesDateTime.UtcNow()and thus cannot work with TimeProvider. That pattern of explicitly usingDateTimeOffset.UtcNow()in places where a TimeProvider should not be used makes it more clear that a future dev should not switch these to TimeProvider.Optional, but a nice addition I think, is a custom attribute that inherits from "FromKeyedServices" attribute,
FromPlatformServices, that makes working and getting the keyed TimeProvider more convenient.Checklist
addedupdated tests, or done manual regression tests