Repository navigation
Add support for .NET5.0 OSPlatformAttribute - #3926
Conversation
8e2b77e to
8efce97
Compare
Dreamescaper
left a comment
Dreamescaper
left a comment
There was a problem hiding this comment.
Generally looks fine by me.
I've only left a minor nitpicks.
| /// </summary> | ||
| /// <param name="possibleSupportedOSPlatformAttribute"></param> | ||
| /// <returns></returns> | ||
| public static object Translate(object possibleSupportedOSPlatformAttribute) |
There was a problem hiding this comment.
Personally I would prefer something like that:
public static bool TryTranslate(object possibleSupportedOSPlatformAttribute, out PlatformAttribute platformAttribute);That would make it more obvious what this method returns.
And then use it smth like that:
OSPlatformTranslator.TryTranslate(attribute, out var platformAttribute) ? platformAttribute : attribute(Your option is more concise though, so I don't insist to hard)
There was a problem hiding this comment.
Because of #3941 I have to changes this, combining multiple OSPlatformAttribute into a single PlatformAttribute
There was a problem hiding this comment.
Thanks for explaining this, I was going to ask why the need to handle PlatformAttribute in the translator. Are you saying that this PR will also fix #3941 ?
There was a problem hiding this comment.
@stevenaw This PR will as a side effect combine multiple Platform attributes into one, thereby 'fixing' #3941. It depends if the owners want that or change the AllowMultiple flag.
The main reason I combine it here is for code that currently use both SupportOSPlatform and Platform. The first for the .NET compiler, the 2nd for nunit.
There was a problem hiding this comment.
@manfred-brands Sorry it's taken me a few days to get back to you. That makes a lot of sense, thanks!
There was a problem hiding this comment.
I think it is fix to fix #3941 and keep the AllowMultiple as it is.
b61af83 to
8d9b37b
Compare
|
@mikkelbu MacOS builds fail with "The repository primary signature validity period has expired". Also fails in 'master'. |
|
Let's ignore the MacOS failures for now. It looks like a problem with the build images being out of date. I can override and merge. |
|
I also noticed the problems with builds which was also why I manually triggered https://nunit.visualstudio.com/NUnit/_build/results?buildId=4256&view=results, but I've not had time to look at it the last couple of week. Regarding failures like this I proposed to run the builds every day in #3928, but so far I've not heard any feedback. EDIT: I've made a PR that should fix the CI on MacOS. |
stevenaw
left a comment
stevenaw
left a comment
There was a problem hiding this comment.
@manfred-brands Thanks for investigating + putting this PR up! I have one suggestion that may help with ensuring these tests continue to run in the future
8d9b37b to
afa2578
Compare
Perform a few compiler suggested changes.
|
Thanks for your contribution @manfred-brands ! We'll need a second approval before the PR can be merged. |
|
LGTM. Thanks for your contribution @manfred-brands ! |
5 participants
Fixes #3866
Approach taken is to convert a
SupportedOSPlatformAttributeinto aPlatformAttributewithIncludeanda
UnsupportedOSPlatformAttributeinto aPlatformAttributewithExclude.Here the passed in PlatformName is translated into a Platform that is supported by NUnit.
Currently only supported are:
Other names are passed verbatim, like: Android, Browser.
These will result in invalid Platform name errors in NUnit.