Add test coverage for platform attribute behaviours by stevenaw · Pull Request #5204 · nunit/nunit

stevenaw · 2026-03-30T20:43:56Z

A doc update PR will also be required before both can be reviewed

manfred-brands

@stevenaw What you test is the behaviour of your ObservableAttribute, not the PlatformAttribute. The latter doesn't use properties to checking on the properties at the fixture and test level tests how NUnit deals with properties there, not how the PlatformAttribute works.

Your ChildTestWithoutAttribute_DoesntExplicitlyInheritPlatforms is wrong.
PlatformAttribute on a (base) Fixture mark the test as non-runnable.

The actual unit test needs to check if the test is runnable based upon the platform passed in.
You can either change the expected result based upon the real platform the test is running on, e.g. if running on Linux, the Windows test should be NotRunnable and vice versa.

Alternatively, to bypass the actual Platform in the PlatformHelper, we need to allow a unit test to change the IsPlatformSupported method behaviour to a controlled one supplied by the test.

One way with your new dictionary is to add a method that adds dummy entries to the PlatformChecks dictionary which you then use in your tests.

stevenaw · 2026-03-31T18:12:10Z

Thanks @manfred-brands

My original comment in #5166 and intent here was not to test the run/skip check itself, but rather the relationship between a fixture and tests with regards to this attribute. In particular, that the configuration applied to a fixture is completely independent of that applied to a test method. The run/skip check is then, of course, derived from that however we already have quite good coverage of those checks here: https://github.com/nunit/nunit/blob/main/src/NUnitFramework/tests/Internal/PlatformDetectionTests.cs That's also why I felt it important to write ChildTestWithoutAttribute_DoesntExplicitlyInheritPlatforms as it captures the case where the platform attribute is only present on the fixture. The ObservablePlatformAttribute was then created as a way to see this more directly as a simple "skip/run" doesn't really capture the nuance of why it was skipped or runnable without being able to compare the Include/Exclude properties which factored into the calculation. They were exposed as properties of the test for visibility as the original attribute instance is short-lived

I can try your suggestion to update the tests to probe more directly for the run/skip check by substituting dummy values into the static PlatformChecks field, however modifying a static field would risk leaking state and potentially make the tests more brittle too

stevenaw · 2026-03-31T18:29:25Z

@manfred-brands I've pushed a commit which I think makes the changes you were asking for. It required disabling parallelization of these tests as well making changes to NUnit itself to remove the readonly modifier on the field so that the copy of it I saved prior to mutating it could be safely restored back into the field at the end

stevenaw · 2026-03-31T23:09:37Z


-        private static readonly Dictionary<string, Func<OSPlatform, bool>> PlatformChecks =
+#pragma warning disable IDE0044 // Add readonly modifier
+#pragma warning disable IDE1006 // Naming Styles


I've left these suppressions in like this temporarily until we confirm if we want to go in this direction. If we went ahead then I'd fix the underlying formatting issue

No I wasn't thinking of completely replacing the dictionary.
More of adding non-standard entries.

That way it won't affect the standard operation

manfred-brands

I have added 3 commits.

Using test specific platforms.
Add test on real platforms.
Add tests using Excludes

Only thing left is the test naming.
Even though the attribute is separately applied to the fixture and the test, if the fixture is not runnable, the test will not be run, regardless of the test attributes.
So the attribute on the fixture is implicitly inherited.

manfred-brands · 2026-04-01T07:33:00Z


-        private static readonly Dictionary<string, Func<OSPlatform, bool>> PlatformChecks =
+#pragma warning disable IDE0044 // Add readonly modifier
+#pragma warning disable IDE1006 // Naming Styles


No I wasn't thinking of completely replacing the dictionary.
More of adding non-standard entries.

manfred-brands · 2026-04-01T07:34:49Z

 namespace NUnit.TestData
 {
-    [ObservablePlatform(Includes = [PlatformNames.DotNET])]
+    [Platform(Includes = [PlatformNames.Win])]


I was thinking of using non-standard platforms not affecting any other test.

manfred-brands · 2026-04-01T07:35:23Z

+        [OneTimeSetUp]
+        public void OneTimeSetUp()
+        {
+            _platformField = typeof(PlatformHelper).GetField("PlatformChecks", BindingFlags.Static | BindingFlags.NonPublic)!;


If we make the field internal we don't have to resort to reflection.

Thanks for your change. I admit I usually try to avoid modifying the system under test just to expose things for testing, but I'm alright with this approach.

manfred-brands · 2026-04-01T07:37:32Z

+            Assert.That(testMethod.RunState, Is.EqualTo(RunState.Runnable));
+            Assert.That(fixture.RunState == RunState.Runnable, Is.EqualTo(allowOS));


General question: Why would a testMethod be Runnable when the fixture is not?

I've always understood this to be an implementation detail. TestSuite and TestMethod both implement ITest, and the IApplyToTest interface in turn applies only to the ITest which it has been decorated with. I understand the framework should then account for these varying RunStates when attempting to run a test method where the suite is "Skipped" though I admit I haven't explicitly checked that condition.

There is a bit more discussion on the nuance in #2936 as it relates to properties of tests. The best single comment I could find there to explain is Charlie's comment here: #2936 (comment)

stevenaw · 2026-04-01T17:40:21Z

Thanks for your changes and approval here @manfred-brands . No concerns from me with what you've pushed either.

stevenaw marked this pull request as draft March 30, 2026 20:44

Add test coverage for platform attribute behaviours

00400de

stevenaw force-pushed the 5168-platform--dontmergeprops-tests branch from 4916196 to 00400de Compare March 30, 2026 20:45

manfred-brands requested changes Mar 31, 2026

View reviewed changes

Respond to feedback

b4ba192

stevenaw marked this pull request as ready for review March 31, 2026 23:07

stevenaw commented Mar 31, 2026

View reviewed changes

manfred-brands added 3 commits April 1, 2026 15:41

Use non-standard names to augment PlatformHelper.

3424252

That way it won't affect the standard operation

Add real os specific tests

7d81438

Add examples of test with Excludes

bb2ba50

manfred-brands approved these changes Apr 1, 2026

View reviewed changes

stevenaw merged commit 9daf3ed into nunit:main Apr 1, 2026
5 checks passed

joeldickson mentioned this pull request Apr 18, 2026

Add files to support AI agent coding #5197

Closed

		Assert.That(testMethod.RunState, Is.EqualTo(RunState.Runnable));
		Assert.That(fixture.RunState == RunState.Runnable, Is.EqualTo(allowOS));

Conversation

stevenaw commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manfred-brands left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenaw commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevenaw commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manfred-brands left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenaw commented Apr 1, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

stevenaw commented Mar 30, 2026 •

edited

Loading

manfred-brands left a comment •

edited

Loading

stevenaw commented Mar 31, 2026 •

edited

Loading

stevenaw commented Mar 31, 2026 •

edited

Loading