serilog-web / classic

[Discontinued] Serilog web request logging and enrichment for classic ASP.NET applications
Apache License 2.0
79 stars 32 forks source link

Make the Logging Module's behavior testable + add unit tests #49

Closed tsimbalar closed 6 years ago

tsimbalar commented 6 years ago

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 around HttpApplication and also tried using HttpContextBase, HttpRequestBase, HttpResponseBase, HttpServerUtilityBase (from System.Web.Abstractions) instead of their notoriously hard to test/mock counterparts good old HttpContext, 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> or Func<HttpContext, bool> and replace them with Action<HttpContextBase> or Func<HttpContextBase, bool>. Those changes are technically binary breaking , but not source code breaking given that HttpContextBase and HttpContext offer 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 :)

tsimbalar commented 6 years ago

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 ?

nblumhardt commented 6 years ago

Looks great! :+1: :100:

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 :-)

tsimbalar commented 6 years ago

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 .

nblumhardt commented 6 years ago

Ready to :shipit: ?

tsimbalar commented 6 years ago

Done ! :)

StephenRedd commented 6 years ago

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:

Dim filterContext = Function(Context As HttpContext) Context.Request.Url.PathAndQuery.StartsWith("/__browserLink") ApplicationLifecycleModule.RequestFilter = filterContext

And changing As HttpContext to As HttpContextBase seems to compile in VS, but when you run the site the Application_Start doesn't actually execute -- at least not fully.

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.

tsimbalar commented 6 years ago

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 C:\Windows\Microsoft.NET\Framework\v2.0.50727\Temporary ASP.NET Files and friends helps ... (there are like 4 versions of this folder, one per combination of 32 or 64 bit and ASP.NET version number)