Coverage does not distinguish between property getters and setters

[jeffanders]

Coverlet doesn't appear to differentiate between property getter and setters when calculating coverage.

For example, given a project with only this simple class:

class MyClass
{
    private bool mandatory;
    public bool Mandatory { get => mandatory; set => throw new NotImplementedException(); }
}

And the test project only has the following test:

[Fact]
public void CheckMandatoryDefault()
{
    var sut = new MyClass();

    Assert.False(sut.Mandatory);
}

The test passes, the property is read via it's getter, the setter is not called (i.e. the exception is not thrown) yet the coverage report says 100% coverage. I expected that the coverage report to show 50% coverage.

For completeness I tried changing the Mandatory property to use the following rather than expression bodies for the getter/setters in case the compiler was emitting some compiler generated attribute for expression bodies but it made no difference:

public bool Mandatory { get { return mandatory; } set { throw new NotImplementedException(); } }

[jeffanders]

I think I have identified the issue. The coverage is limited to line level. If I change that implementation to:

public bool Mandatory 
{ 
   get => mandatory; 
   set => throw new NotImplementedException(); 
}

Then this works as expected. But it is definitely a gotcha, particularly when using expression bodies for getters/setters as they can often fit on one line. Not sure if this is already documented somewhere but if not it is probably worth documenting as it could give a false sense of coverage.

[jeffanders]

While it is probably fair that the line coverage percentage is 100% I still think the method coverage percentage is misleading. I expected getters and setters to be treated as separate methods regardless of how many lines they are implemented over.

I suspected that this wasn't specific to property getters/setters but rather a general limitation of only recognising at most one method per line. I confirmed that:

public void MethodA() { } public void MethodB() { }

and

public void MethodA() { } 
public void MethodB() { }

affect coverage differently. While this MethodA/B example is absurd, I think non-trivial property getter/setters on a single line (provided the implementation is still short) is reasonable and should count as separate methods.

[MarcoRossignoli]

@jeffanders understood your observation, but at the moment the way coverlet generate hits is instrumenting code using pdb(lines of code) and IL, coverlet "links" portion of code reported by pdb(for instance line 10-15) with IL op codes section(ie 10 opcode...)...we record if test hitted that portion inserting a "record call"(static method call) inside that block and we account for that 5 lines. Pdb drive us. We account using % of lines "available" and hitted lines.
I don't know if there are other coverage tools that behaves differently...do you know one?

[jeffanders]

@MarcoRossignoli I am literally just getting started with code coverage and Coverlet looked like a great tool to get started with, so I am not familiar with what other tools can do. I was just genuinely surprised by that behaviour as it was literally THE first test I was writing in Coverlet and was genuinely expecting a 50% coverage.

I think that technical limitation is fine and understandable. Perhaps it just needs to be called out as a known issue or something to be aware of somewhere (i.e. if you have non-trivial property you should put the get and set implementations on separate lines, even if they are very short, to ensure code coverage checks for both).

In my case, I knew I was going to be changing that property to have a more complex implementation later (a single flags based enum for a whole bunch of different bool properties like Enabled, Mandatory, Visible etc etc which means I can fit it all in a single enum value rather than many bool instances in memory. This would involve bit logic code which can be error prone) so I wanted to setup a bunch of baseline tests for default values and checking that setters/getters continue to work properly as I change the underlying implementation. It doesn't seem I would be alone in taking that sort of approach and I am guessing others have simply not even noticed this was occurring and perhaps getting a false sense of code coverage (even if it is only slightly inaccurate).

Given the technical limitations the only other way you could try and address this would be to have a Coverlet Roslyn based analyzer which could look for scenarios like this that are known to give inaccurate Coverage results and it could detect multiple "Methods" on a single line and give a warning about inaccurate coverage analysis. I am in no way suggesting this is a priority for Coverlet, just throwing the idea out there if in future an analyzer is added to Coverlet, this check would be something good to include.

[MarcoRossignoli]

just throwing the idea out there if in future an analyzer is added to Coverlet, this check would be something good to include.

Sure every idea/discussion are welcome here!
We can say that at the moment coverlet account "line by line" as you can see on code https://github.com/tonerdo/coverlet/blob/4d61d296e3bd93d52f5eeaaff5b30bd8c0536c67/src/coverlet.core/CoverageSummary.cs#L7

[jeffanders]

Thanks @MarcoRossignoli for providing the reference to that code, it was enlightening to take a look at how this is being reported and how Coverlet is working at a line level in general.

Now that I have a bit more of an idea of what is going on under the covers, I think an analyzer would be overkill. I have a more straight forward idea. Basically I think Coverlet could do the following:

1. If there are 2 or more methods that share a line number in a document and that line number has any coverage than display a warning on the coverage summary similar to the following:

Warning: Tests cover methods or properties that share a line. Coverage for those lines could be inaccurate. Use the /p:CoverletNoLineAliasing option to exclude shared lines from coverage reports.

2. If the /p:CoverletNoLineAliasing option is supplied then any line shared between 2 or more methods is excluded from the set of covered lines. This will make it clearer to the developer when looking at coverage reports that the line is possibly not fully covered and they should move the "methods" to separate lines to get more accurate coverage.

This could be a little challenging as it appears that:
https://github.com/tonerdo/coverlet/blob/4d61d296e3bd93d52f5eeaaff5b30bd8c0536c67/src/coverlet.core/Instrumentation/Instrumenter.cs#L373
only observes a single method per line and ignores subsequent methods on the same line. We could add a simple Coverlet.Core.Instrumentation.Line.Aliased property which could be set if we have already seen this line before for this file for a different method that will track this and be used to raise the warning/exclude coverage for that line.

Care would need to be taken that changing the output of the Coverlet coverage summary to possibly include a warning is not considered a breaking change (e.g. do consumers attempt to pass the coverage report as part of their build processes and an extra warning showing up could break builds). Also, for the /p:CoverletNoLineAliasing option further consideration would be needed regarding which forms of properties should be considered for the purpose of aliasing. It should, at a minimum, probably respect /p:ExcludeByAttribute. However, even if /p:CoverletNoLineAliasing is not used the warning should probably ignore properties with the various compiler generated attributes.

What do you think?

[MarcoRossignoli]

I understood your idea, we can keep open this issue to understand if there is a community interest on this feature.

[jeffanders]

Is this something you would consider a pull request for? Not sure if I have the time for it any time soon and have only just started to look at the underlying code. But assuming I can find time and can figure out the inner workings appropriately I am willing to try and give back via a pull request.

I was thinking about this a bit more and I believe there is also aliasing issues with lambdas and anonymous delegates. These would be even more difficult to solve, particularly as lambdas often appear on one line as part of a larger statement. However, I confirmed that multiline statement lambdas are considered covered if the method containing them has coverage. They are in effect invisible to Coverlet. For example:

public class AliasTest
{
    public Action GetAction()
    {
        var date = DateTime.Now;
        return () =>
        {
            date = date.AddDays(1);
        };
    }
}

with the following test

[Fact]
public void Test()
{
    var sut = new AliasTest();

    sut.GetAction();
}

yields 100% coverage even though the Action delegate constructed within GetAction is never called.

Interestingly C# 7 local functions do appear to work properly when the function body spans multiple lines. Single line local functions suffer from the aliasing problem. For example the following 2 examples are treated differently from a coverage perspective.

public Action GetActionFromLocalFunction1()
{
    var date = DateTime.Now;
    void LocalFunc()
    {
        date = date.AddDays(1);
    };
    return LocalFunc;
}

public Action GetActionFromLocalFunction2()
{
    var date = DateTime.Now;
    void LocalFunc() { date = date.AddDays(1); };
    return LocalFunc;
}

[SteveGilham]

The sequence point information in the .pdb gives start line/column and end line/column information, which would distinguish the two sections of code in the original report, were it all flowed through to the final report.

However, even when you have unvisited code points correctly recorded in whatever raw file, many reporting tools will take any coverage of a line as complete coverage of that line. It may be because reports are aimed at keeping management with dashboard targets off your back, rather than the engineer wanting to see what isn't being covered.

[MarcoRossignoli]

@SteveGilham you've more experience than me on coverage, do you know if exists around some coverage plat with this level o details?I mean instrumenting runtime+report tool?

Is this something you would consider a pull request for?

@jeffanders for the moment I think is better keep this open and wait for future, also because actually as Steve said above most used reporting tools doesn't support this level of details.

[jeffanders]

@SteveGilham After some digging I can see this is a limitation in the downstream reporting tools (for example the Cobertura format is line based). It seems a reasonable approach given the limitations in consumers of the output.

I still think there is a solution for this for those developers who care about this (i.e. it would only apply the stricter concept of coverage when using /p:CoverletNoLineAliasing). Coverlet already supports differentiating coverage on a single line via branches. I think it is a reasonable interpretation of all of the examples I have given previously on this issue that they represent different "branches" of execution on a single line. Given Coverlet and downstream reporting tools already appear to support multiple branches per line then it seems possible to have Coverlet work this way. I think this would also better solve issues where multiple lambdas appear on a single line which is common.

I haven't looked at the instrumentation code at all and whether individual setters, getters, lambdas, anonymous delegates or local functions are already instrumented or whether it would be easy to consider each of these as branches if they share lines with another method. But thought I would throw the idea out there.

[MarcoRossignoli]

FYI I was working on a lambda coverage issue context #369 (blocked at the moment until we'll be able to run real full instrumentation on testing #420)

[jeffanders]

@MarcoRossignoli There definitely seems to be some overlap (no pun intended) with this issue and #369 and your work on it. It looks like your work already improves the situation for lambdas at the very least (and presumably anonymous delegates). I have subscribed to that other issue and once it is complete/merged I will see if I have time/inclination at that point to circle back to this issue, see how it is affected by your changes and what remains outstanding and whether considering each of the aforementioned items as new branches under a separate command line switch is justified.

Thanks for the discussion thus far, it has been very interesting.

[MarcoRossignoli]

Thanks to all for share ideas/knowledge

[SteveGilham]

@MarcoRossignoli The original NCover 1.x recorded start/end line/colum in its XML report, and for viewing classic NCover reports, the NCoverExplorer tool would show this level of detail

and its coverage percentages are of the sequence points rather than lines (as is the summary emitted by OpenCover -- which followed NCover in emitting sequence point start/end line/column information in the XML -- at the end of its run).

[MarcoRossignoli]

Feel free to re-open if needed.

[pkrukp]

Not reopening, but just wanted to say that I noticed this issue today. I had a property with getter/setter defined on the same line and noticed on report from ReportGenerator based on cobertura files that it listed on the getter.

Looking at "raw" coverage.json files I see that coverage is reported in hierarchical way: file - class - function. There is only one function (the getter) there.

So maybe it'd be possible to report both getter and setter functions in raw coverage.json (both would have the same line), and if there are other formats that don't support such situations, then only those formats would have only the getter information ?