Add SkipOnPlatform attribute to Microsoft.DotNet.XUnitExtensions#7184
Add SkipOnPlatform attribute to Microsoft.DotNet.XUnitExtensions#7184akoeplinger merged 2 commits intodotnet:mainfrom
Conversation
In dotnet/runtime we have a bunch of test assemblies that don't make sense on some platforms, e.g. Browser.
Right now we're skipping them via `[SkipOnMono("reason", TestPlatforms.Browser)]` but there's nothing that inherently ties this to Mono other than the current implementation.
Add a more generic SkipOnPlatform attribute that we can use instead.
|
It seems like this attribute is equivalent as the PlatformSpecific one but being reversed. Does it make sense to add a new one instead of just doing: |
|
Additionally it doesn't have a I personally also don't like the double inversion of "platform specific" + "enum bit flip". I don't feel strongly though so if you think another attribute isn't needed I won't object :) |
|
I wanted to get your take on expanding platformspecific to the assembly level and taking a reason. The reason I was suggesting that was because all our tests when we want to skip say on Windows or Browser we use that approach so I was trying to be consistent with what we have. If we add this attribute we might endup in a combine mode, but I think it is fine adding this new attribute as sometimes the enum bit flip is kind of confusing. |
| { | ||
| public IEnumerable<KeyValuePair<string, string>> GetTraits(IAttributeInfo traitAttribute) | ||
| { | ||
| TestPlatforms testPlatforms = TestPlatforms.Any; |
There was a problem hiding this comment.
This means that if for any reason we can't get the argument or get something else (I don't know how it would happen), we would skip the tests on all platforms. Should we instead initialize to, (TestPlatforms)0?
There was a problem hiding this comment.
This is copied from
SkipOnMonoDiscoverer).
I agree with your suggestion, will change it.
There was a problem hiding this comment.
Right, that is because SkipOnMonoDiscoverer supports skipping on Mono on any Platform (by not providing any test platform).
My intention is to switch instances of |
Sounds good! Thanks for adding this new attribute, makes some things easier and clearer. |
While looking at doing the conversion in dotnet/runtime after dotnet#7184 I noticed that having the reason argument reads weird, it's better to have the platform as the first argument so it reads more like a sentence. E.g. instead of ```csharp [SkipOnPlatform("System.Net.Security is not supported on this platform.", TestPlatforms.Browser)] ``` we have ```csharp [SkipOnPlatform(TestPlatforms.Browser, "System.Net.Security is not supported on this platform.")] ``` Also allow multiple attributes since the skip reason can be quite different between platforms.
While looking at doing the conversion in dotnet/runtime after #7184 I noticed that having the reason argument reads weird, it's better to have the platform as the first argument so it reads more like a sentence. E.g. instead of ```csharp [SkipOnPlatform("System.Net.Security is not supported on this platform.", TestPlatforms.Browser)] ``` we have ```csharp [SkipOnPlatform(TestPlatforms.Browser, "System.Net.Security is not supported on this platform.")] ``` Also allow multiple attributes since the skip reason can be quite different between platforms.
…net#7184) * Add SkipOnPlatform attribute to Microsoft.DotNet.XUnitExtensions In dotnet/runtime we have a bunch of test assemblies that don't make sense on some platforms, e.g. Browser. Right now we're skipping them via `[SkipOnMono("reason", TestPlatforms.Browser)]` but there's nothing that inherently ties this to Mono other than the current implementation. Add a more generic SkipOnPlatform attribute that we can use instead. * PR feedback
While looking at doing the conversion in dotnet/runtime after dotnet#7184 I noticed that having the reason argument reads weird, it's better to have the platform as the first argument so it reads more like a sentence. E.g. instead of ```csharp [SkipOnPlatform("System.Net.Security is not supported on this platform.", TestPlatforms.Browser)] ``` we have ```csharp [SkipOnPlatform(TestPlatforms.Browser, "System.Net.Security is not supported on this platform.")] ``` Also allow multiple attributes since the skip reason can be quite different between platforms.
In dotnet/runtime we have a bunch of test assemblies that don't make sense on some platforms, e.g. Browser.
Right now we're skipping them via
[SkipOnMono("reason", TestPlatforms.Browser)]but there's nothing that inherently ties this to Mono other than the current implementation.Add a more generic SkipOnPlatform attribute that we can use instead.