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

Mark architecture-specific tests with ConditionalFact attributes #65862

Merged
merged 2 commits into from
Feb 25, 2022

Conversation

trylek
Copy link
Member

@trylek trylek commented Feb 24, 2022

This change adds initial provisions for platform detection similar
to library logic to CoreCLRTestLibrary and marks a few
architecture-conditional tests with ConditionalFact attributes.

While I must admit I'm not happy about the IL representation of
the ConditionalFact attributes, it's technically using the same
representation as the library tests do and it only affects a handful
of tests. During consolidation of the remaining tests we'll
continue working on an easier solution even though it may require
diverging further away from Xunit.

I'm not yet removing the CLRTestTargetUnsupported clauses from
the corresponding project files, that can only be done after
switching these tests over to use the merged wrappers (my next
change) as the legacy XUnit wrapper generator (in particular the
test execution script generator) relies on those properties.

Thanks

Tomas

/cc @dotnet/jit-contrib

This change adds initial provisions for platform detection similar
to library logic to CoreCLRTestLibrary and marks a few
architecture-conditional tests with ConditionalFact attributes.

While I must admit I'm not happy about the IL representation of
the ConditionalFact attributes, it's technically using the same
representation as the library tests do and it only affects a handful
of tests. During consolidation of the remaining tests we'll
continue working on an easier solution even though it may require
diverging further away from Xunit.

I'm not yet removing the CLRTestTargetUnsupported clauses from
the corresponding project files, that can only be done after
switching these tests over to use the merged wrappers (my next
change) as the legacy XUnit wrapper generator (in particular the
test execution script generator) relies on those properties.

Thanks

Tomas
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 24, 2022
@trylek trylek added area-Infrastructure-coreclr and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Feb 24, 2022
@ghost ghost assigned trylek Feb 24, 2022
@ghost
Copy link

ghost commented Feb 24, 2022

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

Issue Details

This change adds initial provisions for platform detection similar
to library logic to CoreCLRTestLibrary and marks a few
architecture-conditional tests with ConditionalFact attributes.

While I must admit I'm not happy about the IL representation of
the ConditionalFact attributes, it's technically using the same
representation as the library tests do and it only affects a handful
of tests. During consolidation of the remaining tests we'll
continue working on an easier solution even though it may require
diverging further away from Xunit.

I'm not yet removing the CLRTestTargetUnsupported clauses from
the corresponding project files, that can only be done after
switching these tests over to use the merged wrappers (my next
change) as the legacy XUnit wrapper generator (in particular the
test execution script generator) relies on those properties.

Thanks

Tomas

/cc @dotnet/jit-contrib

Author: trylek
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@ghost
Copy link

ghost commented Feb 24, 2022

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

Issue Details

This change adds initial provisions for platform detection similar
to library logic to CoreCLRTestLibrary and marks a few
architecture-conditional tests with ConditionalFact attributes.

While I must admit I'm not happy about the IL representation of
the ConditionalFact attributes, it's technically using the same
representation as the library tests do and it only affects a handful
of tests. During consolidation of the remaining tests we'll
continue working on an easier solution even though it may require
diverging further away from Xunit.

I'm not yet removing the CLRTestTargetUnsupported clauses from
the corresponding project files, that can only be done after
switching these tests over to use the merged wrappers (my next
change) as the legacy XUnit wrapper generator (in particular the
test execution script generator) relies on those properties.

Thanks

Tomas

/cc @dotnet/jit-contrib

Author: trylek
Assignees: -
Labels:

area-Infrastructure-coreclr

Milestone: -

using System;
using System.Runtime.InteropServices;

public static class PlatformDetection
Copy link
Member

Choose a reason for hiding this comment

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

Can we put this in the TestLibrary namespace instead of the global namespace? I know you'd have to go back and manually update all of the custom attributes, so I understand if you'd rather not do so.

Copy link
Member Author

Choose a reason for hiding this comment

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

No worries, I can easily do that.

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.

For what I understand about this, it LGTM. I'll let @jkoritzinsky make the final call.

@trylek trylek merged commit 572405a into dotnet:main Feb 25, 2022
@trylek trylek deleted the PlatformDetection branch February 25, 2022 20:42
@fanyang-mono
Copy link
Member

fanyang-mono commented Mar 2, 2022

@trylek Introducing TestLibrary.dll has slows down the full AOT Mono test lanes, because there are ~2000 more assemblies needed to be AOT'ed for those lanes and AOT process is slow. The checks in this PR also showed that. I wonder if it is possible to avoid creating these new assemblies or create only one copy of it and put it under Core_Root?

@SamMonoRT
Copy link
Member

Adding a link to timeout caused in aot lanes due to this change : #66083

@ghost ghost locked as resolved and limited conversation to collaborators Apr 2, 2022
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.

5 participants