Mock System Clock for FileLoggerProcessor Tests

[wtgodbe]

Right now these tests don't run if it's close to midnight, so that they don't have to account for the file number wrapping back to 0 in the middle of test execution. This also means we don't have any tests that verify that file names/numbers behave correctly on the new day boundary. We should mock the system clock like SignalR does so we can test his behavior: https://github.com/dotnet/aspnetcore/blob/main/src/SignalR/common/Shared/ISystemClock.cs

aspnetcore/src/Middleware/HttpLogging/test/FileLoggerProcessorTests.cs

Lines 192 to 197 in eb0f414

	var now = DateTimeOffset.Now;
	if (now.Hour == 23)
	{
	// Don't bother trying to run this test when it's almost midnight.
	return;
	}

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

[benedikt257]

I'd like to give this a shot. Here is what I would want to get started doing:

• FileLoggerProcessor uses DateTime.Now, so I'd add an interface ISystemDateTime with a single property Now
• Add an implementation for ISystemDateTime that returns DateTime.Now and initialize a new internal property in FileLoggerProcessor with a new instance of that implementation (as is done in https://github.com/dotnet/aspnetcore/blob/main/src/SignalR/server/Core/src/HubConnectionHandler.cs for instance)
• Adapt the existing test cases to use a fixed date instead of the system time. Remove any catch blocks handling dates rolling over within test executions (and replace them with explicit tests for that behavior where sensible). Remove any return statements currently skipping executions of tests at "almost midnight"
• I'd like to use Moq for mocking the ISystemDateTime implementation inside the tests, but would need to add an InternalsVisibleTo for DynamicProxyGenAssembly2 to the HttpLogging project. I wonder if that is permissible (I see it has been done for different projects already)

Does that sound like what you had in mind to you? Happy to take any suggestions / request for alterations!

[BrennanConroy]

• Adapt the existing test cases to use a fixed date instead of the system time. Remove any catch blocks handling dates rolling over within test executions (and replace them with explicit tests for that behavior where sensible). Remove any return statements currently skipping executions of tests at "almost midnight"

Sounds good 👍

• I'd like to use Moq for mocking the ISystemDateTime implementation inside the tests

What does Moq give you that a simple test implementation of the clock wouldn't give you?

[benedikt257]

What does Moq give you that a simple test implementation of the clock wouldn't give you?

I was eying the brevity Moq provides for setting up sequences of return values which I thought might be useful for tests in which I want to simulate the date changing between writes. On second thought (prompted by your comment) that is probably a bad idea because:

• if the implementation of the class ever changes to fetch a date more than once per message written the tests would break
• explicitly setting up the mock with a new date time within the flow of the test will make the intention of the test clearer to the reader

So, no need for Moq really, agreed.

I will get started on this and raise a PR when done.
Thanks for your help!

[thiseasm]

Hello!

Is this issue still up for grabs?

I would like to contribute a bit since this seems to be a pretty straight forward issue.

[BrennanConroy]

Is this issue still up for grabs?

Yeah, I think it's fine if you pick it up 😃

[thiseasm]

Thank you @BrennanConroy, I will look into it.

[benedikt257]

Hi & sorry, I started on this but then lost sight of it as my day job and family responsibilities took over. Since I haven´t seen anything happen here for a couple of weeks, I felt it would be alright to revisit what I had done back then and open a PR for it. (Hope that is alright with you @thiseasm, really don´t want to step on your toes!)

[thiseasm]

Hi @benedikt257. Since you had already made the changes you did good and opened a PR. I can always find something new to work on.