Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SkipAutoProps to work for records as well. #1139

Closed
wdspider opened this issue Apr 8, 2021 · 13 comments · Fixed by #1159
Closed

Fix SkipAutoProps to work for records as well. #1139

wdspider opened this issue Apr 8, 2021 · 13 comments · Fixed by #1159
Assignees
Labels
with repro Issue with repro

Comments

@wdspider
Copy link

wdspider commented Apr 8, 2021

    public sealed record RouteResponse
    {
        public string Id { get; init; } = string.Empty;
    }

I'm still experiencing that autoprops like these in records are still counting against coverage even though SkipAutoProps is set to true.

I originally was under the impression that #328 would fix it for both classes and records; however, I was told to open this new issue since it doesn't. Please fix so that we don't have to keep manually exclude them via the [ExcludedFroCodeCoverage] attribute.

@Malivil
Copy link

Malivil commented Apr 8, 2021

I don't believe it's related to records. I've think I've seen this happen for non-record properties that have inline assignments.

@MarcoRossignoli
Copy link
Collaborator

Thanks for reporting this, maybe it's not related to records but to inline init.

@MarcoRossignoli MarcoRossignoli added the untriaged To be investigated label Apr 8, 2021
@StefanOssendorf
Copy link

The same goes for one-line declarations like public record Foo(String Prop1, String Prop2). These are counted as not covered eventhough SkipAutoProps is enabled.

@Malivil
Copy link

Malivil commented Apr 9, 2021

I created a repro for this: https://github.com/Malivil/Coverlet1139Repro
See GenerateCoverageReport.bat in the Tests project folder for settings and to generate coverage.

If you add tests that instantiate the uncovered records and class then it all shows as covered but since SkipAutoProps is enabled those should be ignored, right?

@wdspider
Copy link
Author

wdspider commented Apr 9, 2021

I created a repro for this: https://github.com/Malivil/Coverlet1139Repro
See GenerateCoverageReport.bat in the Tests project folder for settings and to generate coverage.

If you add tests that instantiate the uncovered records and class then it all shows as covered but since SkipAutoProps is enabled those should be ignored, right?

The SkipAutoProps should make those tests unnecessary, yes. I noticed that your repo is using coverlet.msbuild while we use coverlet.collector. Therefore, it seems like the issue is more internal rather than in just one of the collection methods.

@Malivil
Copy link

Malivil commented Apr 9, 2021

Yea, that's generally the case. The interior logic is the same regardless of collector method

@daveMueller daveMueller added the with repro Issue with repro label Apr 25, 2021
@daveMueller
Copy link
Collaborator

I will work on this.

@daveMueller daveMueller self-assigned this Apr 26, 2021
@daveMueller
Copy link
Collaborator

I analyzed this and would suggest we make another issue for the one-line declarations record. There are two issues here, skipAutoProps ignoring the inline init auto properties and ignoring the one-line declaration record.
I can confirm that the issues are related but there is one important difference for records. The constructor of the record creates an instance of System.Object and I'm currently not sure if I can just ignore this.
Or to be precise I'm a bit afraid that when I just skip this sequence point I will break something, e.g. for inheritance or so. I have to analyze this a bit more.
What is your opinion on that?

image

@daveMueller
Copy link
Collaborator

I analyzed this a bit more and did both in one PR #1159.

@MarcoRossignoli
Copy link
Collaborator

The constructor of the record creates an instance of System.Object and I'm currently not sure if I can just ignore this.

It's ok...every ref object inherits from System.Object

@daveMueller
Copy link
Collaborator

Yes thanks. I guess whats new for records is that the inheritance from System.Object can have a own sequence point as you can see in the screenshot above. I still can't really think of a reason for this sequence point but I concluded that we can skip it.

@wdspider
Copy link
Author

wdspider commented May 5, 2021

I see that the PR got merged. What's the turn around time for release?

@daveMueller
Copy link
Collaborator

AFAIK there is no fixed time. Looking at the changelog I would assume we are close to the next release.

In the meantime you can consume the nightly https://github.com/coverlet-coverage/coverlet/blob/master/Documentation/ConsumeNightlyBuild.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
with repro Issue with repro
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants