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 incorrect comparison in MatchesTheFilter #112048

Closed
wants to merge 1 commit into from

Conversation

filipnavara
Copy link
Member

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 31, 2025
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-reflection-emit
See info in area-owners.md if you want to be subscribed.

@steveharter
Copy link
Member

@steveharter
Copy link
Member

This does look like an issue, but we should have a test.

@steveharter steveharter added the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 3, 2025
@jkotas
Copy link
Member

jkotas commented Feb 11, 2025

This does look like an issue, but we should have a test.

I do not think it is interesting to have tests for corner cases like this one. This typo is not coming back, and it is not really possible to create tests for all potential typos that the code may have.

@hughbe
Copy link
Contributor

hughbe commented Feb 11, 2025

Isn't it good practice to have regression tests for fixes like this? And if easy to add tests covering the other branches on the path?

@jkotas
Copy link
Member

jkotas commented Feb 12, 2025

Thank you for poking at the test coverage. It made me realize that the current logic has number of other corner-case holes. For example, it will match array types even if their rank differs. We should be able to do a simple type equality check instead, that is a lot simpler and that does not need extensive coverage assuming type equality is tested somewhere else.

Superseded by #112464

@jkotas jkotas closed this Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Reflection.Emit community-contribution Indicates that the PR has been added by a community member needs-author-action An issue or pull request that requires more info or actions from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants