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

Generic attributes lack testing #73735

Closed
MichalStrehovsky opened this issue Aug 10, 2022 · 7 comments · Fixed by #106745
Closed

Generic attributes lack testing #73735

MichalStrehovsky opened this issue Aug 10, 2022 · 7 comments · Fixed by #106745
Labels
area-System.Reflection help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged test-enhancement Improvements of test source code
Milestone

Comments

@MichalStrehovsky
Copy link
Member

We need reflection tests that test generic attributes.

dotnet/linker#2963 made it clear that we don't have any targeted reflection tests for the new scenario in the libraries partition. The newly added test is not a targeted reflection test - it's a S.R.Metadata test.

@MichalStrehovsky MichalStrehovsky added area-System.Reflection help wanted [up-for-grabs] Good issue for external contributors labels Aug 10, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 10, 2022
@ghost
Copy link

ghost commented Aug 11, 2022

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

Issue Details

We need reflection tests that test generic attributes.

dotnet/linker#2963 made it clear that we don't have any targeted reflection tests for the new scenario in the libraries partition. The newly added test is not a targeted reflection test - it's a S.R.Metadata test.

Author: MichalStrehovsky
Assignees: -
Labels:

area-System.Reflection, up-for-grabs

Milestone: -

@buyaa-n
Copy link
Contributor

buyaa-n commented Aug 11, 2022

We have some tests, but might need to improve the coverage

@buyaa-n buyaa-n added this to the Future milestone Aug 11, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Aug 11, 2022
@MichalStrehovsky
Copy link
Member Author

The specific tests that are needed are tests that are using the generic type's T in the signatures (that's the bug we ran into).

So things like:

class FooAttribute<T>: Attribute { public FooAttribute(T value, T[] othervalue) { } }
class BarAttribute<T> : Attribute { public T SomeValue { get; set; } }

The existing tests are not testing much because they don't use the T.

@MichalStrehovsky
Copy link
Member Author

I would definitely at least put it in 8.0 but AFAIK generic attributes are new in C# for .NET 7.0 so we should have testing in place to make sure reflection on them actually works in various scenarios.

Cc @333fred

@buyaa-n
Copy link
Contributor

buyaa-n commented Aug 12, 2022

I would definitely at least put it in 8.0 but AFAIK generic attributes are new in C# for .NET 7.0 so we should have testing in place to make sure reflection on them actually works in various scenarios.

Sure, we will reevaluate all issue during 8.0 planning

@333fred
Copy link
Member

333fred commented Aug 12, 2022

AFAIK generic attributes are new in C# for .NET 7.0

That's correct.

@steveharter
Copy link
Member

Moving to 9; the mono bug linked above unfortunately didn't add a test that would have helped here.

@steveharter steveharter modified the milestones: 8.0.0, 9.0.0 Jul 24, 2023
@steveharter steveharter added the test-enhancement Improvements of test source code label Jul 24, 2024
@steveharter steveharter modified the milestones: 9.0.0, 10.0.0 Jul 24, 2024
koenigst pushed a commit to koenigst/runtime that referenced this issue Aug 21, 2024
- Added a test for GetCustomAttribute to verify that the values of the generic type are correctly initialized.
- Added some theory data to check the equality of generic attributes.

Fixes dotnet#73735
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Aug 21, 2024
jtschuster pushed a commit to jtschuster/runtime that referenced this issue Sep 17, 2024
* Added additional tests for generic attributes.
- Added a test for GetCustomAttribute to verify that the values of the generic type are correctly initialized.
- Added some theory data to check the equality of generic attributes.

Fixes dotnet#73735

* Removed active issue attribute for solved issue 56887.

* Added active issue attribute for solved issue dotnet#56887 because the test is still failing on Mono.

* Update src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Attributes.cs

---------

Co-authored-by: Stefan König <stefan.koenig@zuehlke.com>
Co-authored-by: Buyaa Namnan <buyankhishig.namnan@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Oct 2, 2024
mikelle-rogers pushed a commit to mikelle-rogers/runtime that referenced this issue Dec 10, 2024
* Added additional tests for generic attributes.
- Added a test for GetCustomAttribute to verify that the values of the generic type are correctly initialized.
- Added some theory data to check the equality of generic attributes.

Fixes dotnet#73735

* Removed active issue attribute for solved issue 56887.

* Added active issue attribute for solved issue dotnet#56887 because the test is still failing on Mono.

* Update src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Attributes.cs

---------

Co-authored-by: Stefan König <stefan.koenig@zuehlke.com>
Co-authored-by: Buyaa Namnan <buyankhishig.namnan@microsoft.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Reflection help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants