-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Make tests for FileLoggerProcessor system time independent #37755
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
Make tests for FileLoggerProcessor system time independent #37755
Conversation
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.
I reckon this initialization is kind of redundant here - the first invocation of a write will set the date to the current datetime anyhow when the absolute difference between _today and it is positive.
Kept it because it makes it consistent with the previous implementation though - let me know if you think it should be removed.
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.
I like explicitly initializing it when the Processor starts, rather than when the first message is written (that is, don't remove this)
1646cbf to
89be432
Compare
wtgodbe
left a comment
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.
This looks great, thanks!
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| internal interface ISystemDateTime { | ||
| DateTime Now { get; } |
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.
Add a <summary>:
aspnetcore/src/SignalR/common/Shared/ISystemClock.cs
Lines 8 to 10 in 25a869a
| /// <summary> | |
| /// Retrieves ticks for the current system up time. | |
| /// </summary> |
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.
Done, thanks for the review!
|
CI failures are unrelated and should now be fixed - if you push another commit adding the doc comment, I expect CI will go green. |
|
Thanks @benedikt257! |
Make tests for FileLoggerProcessor system time independent
Adapted tests for
FileLoggerProcessorand its test so that they are no longer system time dependent.Description
Slightly changed the implementation of
FileLoggerProcessorto allow setting a date time provider from tests. I adapted the existing unit tests by taking out any handling of possible date roll-overs during test execution and added a new test explicitly testing for that behavior.Fixes #34807