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

Replacing traits used to skip certain test cases with custom Theory and Fact attributes #8348

Merged
merged 8 commits into from
Jan 31, 2023

Conversation

vlada-shubina
Copy link
Member

@vlada-shubina vlada-shubina commented Jan 24, 2023

Related to #8329

Context

Current approach skipping test cases using traits doesn't skip tests cases when running in VS.
In CI tests are ignored as never existed, without any messages.

Changes Made

Implemented platform specific Theory and Fact attributes
Applied them instead PlatformSpecific and SkipOnTargetFramework traits.
Created LongPathSupportDisabledFactAttribute to handle tests only allowed when long path support is disabled (though those tests never run, see #8349)
Removed attributes and traits related to mono.

Testing

Manually tested in VS to see that the tests are indeed skipped with proper error message.
Checked CI runs too.

Notes

@ladipro
Copy link
Member

ladipro commented Jan 24, 2023

The SDK repo has something similar. I wonder if it wouldn't be worth aligning the names and signatures (or just copying those attributes).
Example: https://github.com/dotnet/sdk/blob/main/src/Tests/Microsoft.NET.TestFramework/Attributes/PlatformSpecificFact.cs

@vlada-shubina
Copy link
Member Author

vlada-shubina commented Jan 24, 2023

The SDK repo has something similar. I wonder if it wouldn't be worth aligning the names and signatures (or just copying those attributes). Example: https://github.com/dotnet/sdk/blob/main/src/Tests/Microsoft.NET.TestFramework/Attributes/PlatformSpecificFact.cs

SDK has most of this attributes too: https://github.com/dotnet/sdk/blob/main/src/Tests/Microsoft.NET.TestFramework/Attributes/WindowsOnlyFactAttribute.cs for example.

I just added the additional message as many tests gave the reason why they are skipped in comment which won't be visible in anywhere else.

The next step will be to try to contribute to arcade with same attributes.

If those are approved, they can be used from any repo using the arcade: sdk / installer / msbuild, and others.

Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

Ah, I'm blind.

That's a great plan. Although I'm still finding it odd that this functionality is not already in arcade. We (and the SDK) cannot be the first teams hitting this. Is there a way to make the tests honor the existing traits outside of build -test? Whether they show as skipped or are completely filtered is not a big deal, IMO.

src/Xunit.NetCore.Extensions/Utilities.cs Outdated Show resolved Hide resolved
@vlada-shubina
Copy link
Member Author

Ah, I'm blind.

That's a great plan. Although I'm still finding it odd that this functionality is not already in arcade. We (and the SDK) cannot be the first teams hitting this. Is there a way to make the tests honor the existing traits outside of build -test? Whether they show as skipped or are completely filtered is not a big deal, IMO.

Traits is meant to be used for categorization; and not skipping the tests. Test Explorer UX is built around this: filtering and grouping by traits.

The "skipping" features based on traits in arcade were implemented long ago, note that they also have custom XUnit runner. The feature this PR is using is quite new: xunit/xunit#2073. I believe it's just legacy way of doing it as xunit/xunit#2073 was not available at the time and there was a need for this feature.
I can ask people around if they know anything about this part.

imo, the reason why it is implemented in each repo separately simply as it's very easy to do it; but i agree it's not a great practice.

@vlada-shubina vlada-shubina marked this pull request as ready for review January 24, 2023 17:13
@vlada-shubina vlada-shubina changed the title [Draft] Replacing traits used to skip certain test cases with custom Theory and Fact attributes Replacing traits used to skip certain test cases with custom Theory and Fact attributes Jan 24, 2023
@vlada-shubina
Copy link
Member Author

The tests marked with LongPathSupportDisabledFactAttribute are never run on CI, and locally in most case too.
They need Windows OS where long path support is disabled, but it's not the case for CI.

Created issue to address that: #8349

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Please merge instead of squashing (if we wind up needing to back this out for some reason I don't want to have to lose the wonderful deletion of mono attributes :))

src/Build.UnitTests/Definition/ToolsetReader_Tests.cs Outdated Show resolved Hide resolved
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Thanks!

@vlada-shubina vlada-shubina added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jan 30, 2023
@MichalPavlik MichalPavlik merged commit 6844693 into dotnet:main Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants