Add test logging helpers to Hosting #1005
Conversation
| <PackageReference Include="Microsoft.Extensions.PlatformAbstractions" Version="$(AspNetCoreVersion)" /> | ||
| <PackageReference Include="Microsoft.Extensions.Process.Sources" Version="$(AspNetCoreVersion)" PrivateAssets="All" /> | ||
| <PackageReference Include="Microsoft.Extensions.RuntimeEnvironment.Sources" Version="$(AspNetCoreVersion)" PrivateAssets="All" /> | ||
| <PackageReference Include="Microsoft.NETCore.Windows.ApiSets" Version="$(WindowsApiSetsVersion)" /> | ||
| <PackageReference Include="Serilog.Extensions.Logging" Version="1.4.0" /> | ||
| <PackageReference Include="Serilog.Sinks.File" Version="3.2.0" /> |
There was a problem hiding this comment.
Does @Eilon know you're using these versions
There was a problem hiding this comment.
Even these dependency versions go in the dependencies.props
There was a problem hiding this comment.
Yes, they were in the original ServerTests code that this is ported from.
There was a problem hiding this comment.
@Eilon sorry if they need to be re-submitted when moved like this.
There was a problem hiding this comment.
From an OSS perspective if it's the same version it's fine.
From a product perspective, make sure the lead/PM.architect for this area agrees that adding these dependencies makes sense (because it affects everyone using the package, of course).
There was a problem hiding this comment.
Makes total sense. This is an internal (noship) package, so the impact should be minimal.
| @@ -5,7 +5,7 @@ | |||
| <PropertyGroup> | |||
| <Description>ASP.NET Core helpers to deploy applications to IIS Express, IIS, WebListener and Kestrel for testing.</Description> | |||
| <VersionPrefix>0.4.0</VersionPrefix> | |||
| <TargetFrameworks>net46;netstandard1.3</TargetFrameworks> | |||
| <TargetFrameworks>net46;netstandard1.5</TargetFrameworks> | |||
There was a problem hiding this comment.
Assembly.GetEntryAssembly
There was a problem hiding this comment.
And since these are only used in tests which target netcoreapp1.1 and we don't ship the package, this is a fully compatible change (though I will be confirming that before merging)
| @@ -0,0 +1,153 @@ | |||
| using System; | |||
|
🆙 📅 (and one that adds new reviewable code) - ServerTests also had an HttpClient handler that logged requests which seemed very useful for testing. I moved it to this repo and added a way to ensure that tests can always get an HttpClient with LoggingHandler, pointed at the deployed site. These changes came up as I was testing ServerTests with this new package. They aren't necessary to merge, but remove more duplicate code :) |
| protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) | ||
| { | ||
| _logger.LogDebug("Sending {method} {url}", request.Method, request.RequestUri); | ||
| var response = await base.SendAsync(request, cancellationToken); |
There was a problem hiding this comment.
yeah, not a bad idea at all :)
These are based off the ones in ServerTests. Now that I've seen them at work, I want to add these to Entropy because we're seeing some hanging tests, but I don't want copy-pasta so I figured I'd make it common in Hosting.
I do plan to make this even more low-impact by adding some xunit extensibility to make it "magic", but I don't want to do that until I've worked out the logging process on a few tests.
There's a particular spot in here I'm not so proud of, but very open to suggestions ;). See if you can spot it!
Note: At some point I plan to pull this to somewhere even more common (Common?) in order to be able to use it in all our tests. Also, I plan to use some xunit hooks to avoid some of the boilerplate.