Invalid platform name passed to PlatformAttribute should mark test NotRunnable

[ChrisMaddock]

From #2200 - if an invalid platform name is passed into PlatformAttribute, the test is currently skipped - the same as if a platform the test is not currently running on was passed in.

An invalid platform name would always be a mistake, so in my opinion should mark the test NotRunnable, and fail the test run.

Thoughts?

[CharliePoole]

Agree on included platforms, what about excluded?

[ChrisMaddock]

I think this should cover all lists. Excluding an invalid platform would currently skip the test on all platforms, which is potentially a hard-to-spot, wrong-side failure.

[CharliePoole]

I thought excluding an invalid platform caused it to run. I'm on my phone so I can't check.

[ChrisMaddock]

I haven't tried, it, but it looks like anything invalid throws an ArgumentException, which is caught by IsPlatformSupported(), which then returns false.

nunit/src/NUnitFramework/framework/Internal/PlatformHelper.cs

Line 135 in 077aa75

catch (Exception ex)

Either way, I still would prefer an invalid test, over the test running on a platform I didn't intend it to. 😄

[jnm2]

My opinion on correctness issues is always that it should fail as soon and as clearly as possible. 😄

[ChrisMaddock]

Relabelling this as an easyfix enhancement.

PlatformHelper currently throws and catches an Exception, for invalid framework names. In my opinion, that exception should instead be caught within PlatformAttribute, which can then mark the test as NotRunnable, with an appropriate reason.

[david-proctor]

Is this still up for grabs? I'd like to take it if so.

[ChrisMaddock]

Great - it's yours! Thanks!

[rprouse]

@david-proctor I've sent you an invite to the contributors team so that we can assign this issue to you. Thanks for the help.

[david-proctor]

@rprouse Thanks! I've put a commit together but I don't think I have push access to the repo. If someone could grant that I'll start a pull request.

[rprouse]

@david-proctor normally we start new contributors out working in forks of the repo to get started, but since you are Canadian (joking) I will temporarily add you to the framework team. We would normally add you after a couple of PRs and activity in the issues, so if you are planning on sticking around I will leave you on the framework team.

Master is locked, so no worries about messing up. If we decide to keep you on as a member, your PRs will only require one other team member to review going forward and you are welcome to review and merge other PRs. Our policy is for two team members to review non-team PRs and one team member to review other team members PRs. Because this is your first PR, we will still require two reviews.

If you have any questions now or at any time, please email me at rob@prouse.org.

Welcome 😄

[david-proctor]

Thanks a lot, @rprouse. I've started the pull request #2314.

[david-proctor]

Pull request #2314 has been merged, so I think this issue is resolved. Thanks for the assistance, everyone.

[jnm2]

@david-proctor Thank you for the contribution!