Additional coverage reporting for properties and nested types #68
Conversation
Minor refactorings elsewhere: * Inline functions to local functions. * Extract method for obtaining backup module path. * Avoid hard-coding coverlet assembly name.
|
@hunterjm Yeah that's true. I'm fine with merging as your changes look to be good improvements too. |
|
@hunterjm you're gonna have a lot of merge fun then 😛. This PR seems straightforward enough, need to review yours over coffee |
|
That's fine @tonerdo, I'll keep an eye on this PR and when it get's merged I'll update mine. |
| @@ -95,7 +133,7 @@ private void InstrumentIL(MethodDefinition method) | |||
| { | |||
| var instruction = processor.Body.Instructions[index]; | |||
| var sequencePoint = method.DebugInformation.GetSequencePoint(instruction); | |||
| if (sequencePoint == null || sequencePoint.StartLine == 16707566) | |||
| if (sequencePoint == null || sequencePoint.StartLine == HiddenSequencePoint) | |||
There was a problem hiding this comment.
I would just use !sequencePoint.IsHidden instead of checking line numbers. Cecil already provides this info for us (and it basically does just that).
| InstrumentMethod(method); | ||
| } | ||
|
|
||
| foreach (var property in type.Properties) |
There was a problem hiding this comment.
The C# compiler generates methods for properties. Those methods are returned as part of type.Methods, no need to explicitly instrument properties
There was a problem hiding this comment.
Thanks. You're right. I've removed enumeration of property methods.
|
|
||
| // Restore the original module - retry up to 10 times, since the destination file could be locked | ||
| // See: https://github.com/tonerdo/coverlet/issues/25 | ||
| var currentSleep = 6; | ||
| Func<TimeSpan> retryStrategy = () => { | ||
| TimeSpan retryStrategy() |
There was a problem hiding this comment.
Since the retry strategy is the same in InstrumentationHelper.cs, we can just create a single method and use that everywhere
| if (type.HasNestedTypes) | ||
| { | ||
| foreach (var nestedType in type.NestedTypes) | ||
| InstrumentType(nestedType); |
There was a problem hiding this comment.
Did you confirm that this actually fixes the specified issues. It was my thinking that Cecil returned every type regardless of whether they were nested or not
There was a problem hiding this comment.
When I was adding the branch logic, I saw something similar in OpenCover's symbol manager: https://github.com/OpenCover/opencover/blob/35f1299e136c9f91241839b85e20d7eb6ed24416/main/OpenCover.Framework/Symbols/CecilSymbolManager.cs#L183
There was a problem hiding this comment.
Works for me. @sjp feel free to ignore this comment
There was a problem hiding this comment.
Sure. I've just changed the logic slightly to use module.Types instead of module.GetTypes() so that this logic for nested types can be retained without processing nested types more than once.
There was a problem hiding this comment.
@sjp - Actually, I think @tonerdo is correct. GetTypes() from Mono.Cecil already handles nested types. He was also correct that Properties were created as methods as seen in the attached screenshot (all that went up was hit count when properties were added).
I actually would kind of prefer letting Cecil handle type nesting.
| if (type.HasNestedTypes) | ||
| { | ||
| foreach (var nestedType in type.NestedTypes) | ||
| InstrumentType(nestedType); |
There was a problem hiding this comment.
@sjp - Actually, I think @tonerdo is correct. GetTypes() from Mono.Cecil already handles nested types. He was also correct that Properties were created as methods as seen in the attached screenshot (all that went up was hit count when properties were added).
I actually would kind of prefer letting Cecil handle type nesting.
Additional coverage reporting for properties and nested types
This should fix #7 and #39.
Nested types were not being covered, which is probably why some LINQ code and anonymous types were not being marked as covered.
In addition to this I've also modified
ExcludeFromCodeCoverageAttributeso that it can be applied to properties.There have also been some other minor refactorings:
namespaceFinally, I've improved performance by caching some reflection that retrieves
CoverageTracker.MarkExecuted(). This should be a one-off operation instead of (potentially) once per sequence point.