search

serilog / serilog-settings-configuration

A Serilog configuration provider that reads from Microsoft.Extensions.Configuration
Apache License 2.0
444 stars 129 forks source link

Improve test cases for ObjectArgumentValue #409

Closed ChristofferGersen closed 6 months ago

ChristofferGersen commented 7 months ago

This also includes some improvements for StringArgumentValueTests.

This pull request contains only the test cases that succeed without the intended changes of pull request https://github.com/serilog/serilog-settings-configuration/pull/405. Merging this before https://github.com/serilog/serilog-settings-configuration/pull/405 is meant to ensure that https://github.com/serilog/serilog-settings-configuration/pull/405 does not break anything. After this is merged, I will do the following for the other pull request:

  1. Rebase the other pull request
  2. Add test cases for the intended changes of that pull request (I already have these test cases on my local system)
  3. Refactor ObjectArgumentValue to remove use of Linq expressions and have TryBuildCtorExpression call ObjectArgumentValue.ConvertTo recursively, instead of itself. I have already tested this locally and it works well for all test cases added here.

The remaining question is whether these test cases are sufficient to make such an impactful change.

nblumhardt commented 7 months ago

The added tests look great.

I am unsure about (3), removing the LINQ expression binding mechanism, because I can see the advantages there for declarative testing and debugging. At first glance, the old tests written in that style seem nice and clear.

Would it be possible to outline the reasoning for that change some more? I think it needs @0xced's eyes on it.

ChristofferGersen commented 7 months ago

The main problem I have with the Linq expressions, is that it currently requires two implementations for every feature (like array/dictionary support). You can avoid this by calling ObjectArgumentValue.ConvertTo inside TryBindToCtorArgument and wrapping the result inside a Expression.Constant, just like is done with the results of StringArgumentValue.ConvertTo. In the end this will result in an Expression.New with a bunch of Expression.Constant arguments. This is not that different from what I propose, since I just invoke the constructor directly, without going through Linq expressions.

Just to demonstrate what I would like the code to become I have created another branch that is based on the branch from https://github.com/serilog/serilog-settings-configuration/pull/405. See the relevant commit at ChristofferGersen/serilog-settings-configuration@4d7308aac159cdbaee406e1980850a1be7ea16cd. The interesting part is the removal of TryBindToCtorArgument inside ObjectArgumentValue. Doing so makes the implementation much simpler in my opinion. And it also ensures that everything that ObjectArgumentValue supports, is also supported when explicitly specifying the type inside appsettings.

You also mentioned you liked how the test cases looked using Linq expressions. For simple cases it does look fine, but Expression.Constant is displayed by using ToString on the value. This is okay for numbers and strings, but is less meaningful for collections. In my previous test cases those also looked somewhat okay, but that is because I created additional implementations for CreateArray and TryCreateContainer that used more specific Linq expresssions (Expression.NewArrayInit and Expression.ListInit). This is what I want to avoid.

0xced commented 6 months ago

Adding new tests first and opening a pull request only about those tests was a great approach, easier for reviewing, thanks! The new tests are good, I just did some minor code cleanup.

I'm waiting for AppVeyor to finish running the tests then I'll merge so that you go ahead with #405. 😉