coverlet-coverage / coverlet

Cross platform code coverage for .NET
MIT License
2.93k stars 385 forks source link

Treat records equal to classes #1576

Open daveMueller opened 6 months ago

daveMueller commented 6 months ago

This is a proposed design change related to records. As records in the IL are just classes with some generated methods we should also treat them like classes. With issue #1139 we implemented records (with primary constructor) to be completely excluded from instrumentation when auto properties are skipped. This is somehow troublesome because in the IL there is no difference to classes or records with auto properties. As an example, when we SkipAutoProps the instrumentation of a record with a primary consturctor is completely skipped and looks like this:

grafik

If we now compare this to an equivalent class with the same coverage parameters, the assignement in the constructor is still beeing instrumented (even with SkipAutoProps).

grafik

I would suggest to do the same for records. As assignement in the constructor and definition of properties is all part of the primary constructor, it also should be instrument even if auto properties are skipped.

grafik

tengl commented 5 months ago

I guess the same would apply for records with default constructor, like this:

public record SomeType
{
    public string? Optional { get; init; }
    public required string Required { get; init; }
}

If I change to class, I get 100 % coverage, but with record I get 66% coverage.

daveMueller commented 5 months ago

@tengl This is another issue we already fixed with https://github.com/coverlet-coverage/coverlet/pull/1575. The roslyn team changed something in the compiler with the release of net8. They emit additional sequence points for records to get a better debugging experience etc... You can try to consume our nightly build to get coverage back to 100%.

grafik

tengl commented 4 months ago

@daveMueller that's great, I'll wait for the NuGet update.

daveMueller commented 4 months ago

@MarcoRossignoli @bert2 what do you think about this. I think we would have less issues with records if we would undo the changes we did for #1139.

MarcoRossignoli commented 4 months ago

Fine to me

github-actions[bot] commented 1 month ago

This issue is stale because it has been open for 3 months with no activity.