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.

Sign up for GitHub

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

Jump to bottom

Wrong coverage for await foreach with generic method #1312

Merged
merged 1 commit into from
Mar 15, 2022
Merged

Wrong coverage for await foreach with generic method #1312

merged 1 commit into from
Mar 15, 2022

Conversation

daveMueller
Copy link
Collaborator

@daveMueller daveMueller commented Mar 15, 2022 •
edited

closes #1210

grafik

@daveMueller
Copy link
Collaborator Author

@MarcoRossignoli Can you run the build again? The failed test has nothing to do with my changes. The job already was green before I updated the changelog.

@MarcoRossignoli
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

MarcoRossignoli
MarcoRossignoli approved these changes Mar 15, 2022
View reviewed changes
Copy link
Collaborator

@MarcoRossignoli MarcoRossignoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot ❤️

@daveMueller
Copy link
Collaborator Author

daveMueller commented Mar 15, 2022 •
edited

What I've analysed is that in the case of generics, the operand isn't a FieldDefinition. So every symbol check that is looking for a FieldDefinition will not match. There are places in CecilSymbolHelper where this is already considered, e.g. https://github.com/coverlet-coverage/coverlet/blob/master/src/coverlet.core/Symbols/CecilSymbolHelper.cs#L114-L115. But there are also a couple of places where it isn't.
In this PR I only changed the necessary places to fix the bug but maybe it would make sense to change this everywhere in CecilSymbolHelper?

@MarcoRossignoli
Copy link
Collaborator

MarcoRossignoli commented Mar 15, 2022 •
edited

I think it's ok as-is for now, if we don't have feedback.
If you feel safe to update also around go for it.
We have a pretty good coverage I think.

@MarcoRossignoli MarcoRossignoli merged commit 3c1124e into coverlet-coverage:master Mar 15, 2022
@daveMueller daveMueller deleted the 1210_AwaitForEachGeneric branch March 15, 2022 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarcoRossignoli MarcoRossignoli MarcoRossignoli approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wrong coverage for "await foreach" when method is generic
2 participants
@daveMueller @MarcoRossignoli