Obsolete CancelledTestNodeStateProperty

[Copilot]

Obsolete CancelledTestNodeStateProperty - COMPLETE ✅

Based on the issue discussion and agent instructions, the CancelledTestNodeStateProperty should not have been introduced. When cancellation is requested, test frameworks should throw OperationCanceledException using the cancellation token passed by Microsoft.Testing.Platform, rather than reporting a cancelled test state.

All changes completed:

• Add [Obsolete] attribute to CancelledTestNodeStateProperty class in TestNodeProperties.cs with clear guidance message
• Keep CancelledTestNodeStateProperty in WellKnownTestNodeTestRunOutcomeFailedProperties and WellKnownTestNodeTestRunOutcomeProperties arrays with suppressed obsolete warnings
• Add pragma warning suppressions for ALL occurrences of CancelledTestNodeStateProperty in the codebase
• Code review feedback fully addressed
• Code review passed - no issues found
• Security scan passed - no vulnerabilities detected

Files updated with pragma warning suppressions:

• TestNodeProperties.Categories.cs (2 occurrences)
• HangDumpActivityIndicator.cs (1 occurrence)
• MSBuildConsumer.cs (1 occurrence)
• TerminalOutputDevice.cs (1 occurrence)
• BrowserOutputDevice.cs (1 occurrence)
• DotnetTestDataConsumer.cs (1 occurrence)
• Json.cs (1 occurrence)
• PerRequestServerDataConsumerService.cs (1 occurrence)
• SerializerUtilities.cs (1 occurrence)
• AzureDevOpsReporter.cs (1 occurrence)
• TestExecutionContext.cs (1 occurrence - MSTest.Engine)
• TestApplicationResultTests.cs (1 occurrence)
• TestNodePropertiesTests.cs (already has file-level suppression)

Impact:

• ✅ CancelledTestNodeStateProperty is marked as obsolete to discourage its use
• ✅ All internal platform code suppresses the warning for backward compatibility
• ✅ External test frameworks will see obsolete warnings with clear guidance
• ✅ No build warnings in the platform itself

Security Summary:
No security vulnerabilities introduced or identified in the changes.

Original prompt

---

This section details on the original issue you should resolve

<issue_title>Cancelled tests not reported as failures</issue_title>
<issue_description>See this run: https://github.com/thomhurst/ModularPipelines/actions/runs/12072550368/job/33666771665?pr=787#step:9:3473

Cancelled tests are marked as failures.

I'd argue they shouldn't be counted as failures as that's misleading. They haven't failed per se, they were just terminated.

If the overall test run was cancelled it should fail overall, but not each cancelled test imo.

What do you think?</issue_description>

<agent_instructions>Obsolete the cancelled test node state.</agent_instructions>

Comments on the Issue (you are @copilot in this section)

@Evangelink Is this functionally expected to have a test being cancelled? For me, it is an error even if that's the result of a user requesting cancellation.

We cannot say the test is ok as it didn't reach the end of its action.</comment_new>
<comment_new>@Evangelink
I understand the TE situation, let's put it on the side for a second to focus on CLI experience, what would be your expectations there?</comment_new>
<comment_new>@nohwnd
Sounds reasonable to me to not report it as error (there is no additional info in the error anyway?) as long as we guarantee that the whole run will finish with error code.

@Youssef1313 > The first failure will cancel the test session's cancellation token, and so all pending tests would then be marked as cancelled. Sifting through the output/test explorer to find that failure is harder if all the cancellations look like failures too.

Why not simply skip reporting any test results for all the pending tests in this case?</comment_new>
<comment_new>@Youssef1313
My thought after revisiting this, I feel like we shouldn't have introduced a cancelled test node state altogether.

If cancellation is requested (e.g, via Ctrl+C), the test framework should just call cancellationToken.ThrowIfCancellationRequested(), which then propagates up as cancellation.

@thomhurst Is there any reason you really need this state? If others agree, I would personally consider marking it as obsolete.

• We don't use this state in the VSTestBridge (so both MSTest and NUnit don't use it).
• xUnit.net doesn't seem to also use that state (cc @bradwilson for confirmation)
• I don't see exactly how this is reachable in TUnit. I see usage here, but I don't see anything that sets TestState.Cancelled. @thomhurst Can you clarify please how you are using that? There is another usage here, but it says it shouldn't happen (should you actually throw UnreachableException so it becomes more clear this is not supposed to be reached?)
• MSTest.Engine (the experimental source gen based MSTest is using it, but I don't think it's correct to use that state).</comment_new>
  <comment_new>@Youssef1313
  For timeout, it makes sense to me to end up as a failure.
  For cancelled, the whole state, IMO, shouldn't have existed. I'm trying to understand more how/why you need a cancelled state. When a test run is cancelled, I think you should just throw and let the flow back to MTP to finalize any needed cleanups and exit with aborted exit code.</comment_new>
  <comment_new>@Youssef1313

In regards to throwing and bubbling back to MTP, that doesn't really work. You get unclear errors like Test framework reported a start event but not an end event (or something like that, can't remember off the top of my head.)

Can you have more clear repro of what goes wrong when you throw cancellation and let the flow back to MTP? I would prefer to address that instead, and then obsolete the cancelled state.</comment_new>
<comment_new>@Evangelink
MTP currently catches cancellation exceptions if the token on the exception matches the token of MTP execution be wise that's the only way to be sure this is a cancellation requested by MTP. Your example although throwing Oce is not something triggered by MTP so it's considered like any other exception.</comment_new>
<comment_new>@Youssef1313
Exactly. Cancellation should happen via cancellationToken.ThrowIfCancellationRequested(), and only using the cancellationToken that MTP passes to the framework.</comment_new>

• Fixes Cancelled tests not reported as failures #4196

---

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

[Youssef1313]

@copilot Any occurrence of CancelledTestNodeStateProperty in the codebase should be surrounded by #pragma warning disable CS0618 // Type or member is obsolete and #pragma warning restore CS0618 // Type or member is obsolete

[Copilot]

@copilot Any occurrence of CancelledTestNodeStateProperty in the codebase should be surrounded by #pragma warning disable CS0618 // Type or member is obsolete and `#pragma warning restore CS0618 // ...

Done. Added #pragma warning disable CS0618 and #pragma warning restore CS0618 around all occurrences of CancelledTestNodeStateProperty in the codebase (13 files total). (commit a144a8f)

[Evangelink]

I am not opposed to that obsolescence but I am curious as to why? The original design was to say we have 2 parent states success and failure and sub-states from these states allowing to define more precisely the kind of issue so that clients (e.g. Console, IDE...) can make specialized display.

[Youssef1313]

I don't see any possible meaningful usage of CancelledTestNodeStateProperty. Happy to discuss if you have examples though.