Make the Logging Module's behavior testable + add unit tests #49
Conversation
…EventHandler + apply Resharper hints / use C# 7 goodies when possible + new Test project to test around current behavior (WIP) + added ApplicationLifecycleModule.ResetConfiguration() to ease testing *BREAKING CHANGE* : the callbacks now take a HttpCo+ ntextBase / HttpRequestBase instead of a HttpContext/HttpRequest
|
The build initially failed because it targeted an older version of C#... If the changes here are gonna cause an increment in the major version of the package, I think I might also as well reference Serilog v2.6 instead of v2.5 as well, right ? |
appveyor.yml
Outdated
| @@ -1,4 +1,5 @@ | |||
| version: '{build}' | |||
| image: Visual Studio 2017 | |||
| build_script: | |||
| - ps: ./Build.ps1 -majorMinor "3.0" -patch "$env:APPVEYOR_BUILD_VERSION" -customLogger "C:\Program Files\AppVeyor\BuildAgent\Appveyor.MSBuildLogger.dll" | |||
There was a problem hiding this comment.
Given the binary breaking change, should we bump this to 4.0?
|
Looks great! 👍 💯 Targeting Serilog 2.6 sounds like the way to go. I'm AFK most of the weekend, so might not get back to this for a few days, please go ahead and press the big green button without me if you are ready to merge :-) |
|
There is no hurry :) I'd rather wait to merge it until Monday anyway .... it's the week-end after all and I'd rather not break things down the line if I'm not around to fix it :p . I'll fix and merge on Monday if you haven't by then . |
|
Ready to |
?
|
Done ! :) |
|
I haven't fully researched this, but updating to the newer version breaks some of my Webform applications (VB). This is an excerpt from Application_Start in Global.asax.vb, and it doesn't compile after the update:
And changing This application is rather huge, and poorly behaved... I'll see about getting a smaller example project setup for further testing to see if I can get a better idea what's actually going on, but I wanted to go ahead and point out that this is a source breaking change -- at least in some cases. |
|
That's strange :-/ We knew from the start it would not be binary-compatible (hence the major version bump 3.0 -> 4.0), but it didn't break apps where I tested it. It may be some oddity of IIS / ASP.NET that caches a bit too aggressively ... somethomes cleaning the "Temporary ASP.NET Files" folders under |
This project currently has no unit tests, and instead had a sample WebForms project.
I would like to do some refactoring in SerilogWeb.Classic later on, but that seemed a bit risky without having a decent coverage of all the combinations of configuration that can be set on
ApplicationLifecycleModule. I therefore added a new test project :SerilogWeb.Classic.Tests.In order to add unit tests, I had to change a few things to make it testable (duh). I tried to extract all of the logic that was currently in the HttpModule and move it to a separate less infrastructure-coupled class
ClassicRequestEventHandler. I introduced some kind of wrapper aroundHttpApplicationand also tried usingHttpContextBase,HttpRequestBase,HttpResponseBase,HttpServerUtilityBase(fromSystem.Web.Abstractions) instead of their notoriously hard to test/mock counterparts good oldHttpContext,HttpRequest,HttpResponse,HttpServerUtility.I could leave most of the logic untouched, but I had to introduce changes on the "configuration parts" that accept an
Action<HttpContext>orFunc<HttpContext, bool>and replace them withAction<HttpContextBase>orFunc<HttpContextBase, bool>. Those changes are technically binary breaking , but not source code breaking given thatHttpContextBaseandHttpContextoffer a compatible public surface area. I believe that would still call for a major version dump though...(Also I couldn't resist introducing minor refactorings mostly suggested by ReSharper in parts where it made the code more readable or less verbose)
In the current state, the public API of SerilogWeb.Classic is therefore changed only in the signature of the delegates that could be set in the configuration. No public members have been removed or added though, and the configuration still goes through
ApplicationLifecycleModule's public static properties, although I would like to change that in the future, but that'll be another story :)