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

Add [Fact] attributes to all remaining ilproj tests #61625

Merged
merged 3 commits into from
Nov 17, 2021

Conversation

trylek
Copy link
Member

@trylek trylek commented Nov 15, 2021

No description provided.

@ghost
Copy link

ghost commented Nov 15, 2021

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: trylek
Assignees: -
Labels:

area-Infrastructure-coreclr

Milestone: -

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

If we ever revise the publickeytoken/version of xunit.core, it's going to be a tremendous pain. Do we even need to specify those? It looks like there is no version specified for System.Console, System.Runtime, or mscorlib in these IL tests.

@trylek
Copy link
Member Author

trylek commented Nov 15, 2021

@jkoritzinsky, can you please comment? I basically took what you have in the POC PR but I'm all for reducing longer-term maintenance pain.

@jkoritzinsky
Copy link
Member

I'll go validate whether or not we need the version and public key token info for the test to be runnable and circle back here.

@jkoritzinsky
Copy link
Member

So fun discovery: Since we never actually use the reflection APIs to look up the attribute, the runtime never even tries to resolve the reference. If we wanted to, we could use whatever assembly name we wanted here as it's never validated at any point. In any case, based on how mscorlib is resolved by the runtime without the version or public key token, I think we can remove them for the xunit.core reference as well and the loading will work if any of the IL tests move to use any other Xunit APIs.

@trylek
Copy link
Member Author

trylek commented Nov 16, 2021

Great to know, I'll update this PR by removing the assembly details and I'll prepare another PR removing them from those ilproj tests I have already switched over, thanks Jeremy for verifying!

@trylek
Copy link
Member Author

trylek commented Nov 17, 2021

Merging in with one unrelated failure in library tests that are unaffected by my change.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants