Deprecate EnableShutdownAfterTestRun which is no-op#15576
Conversation
There was a problem hiding this comment.
Pull request overview
Deprecates the EnableShutdownAfterTestRun framework handle property (now a no-op) and removes engine usage of the flag so it can’t influence execution behavior.
Changes:
- Marked
EnableShutdownAfterTestRunas obsolete (compile-time error) on interface and implementations. - Removed engine read-path for
EnableShutdownAfterTestRunduring runs. - Removed now-irrelevant unit tests validating the property’s previous behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Adapter/FrameworkHandleTests.cs | Removes tests that asserted the property’s previous getter/setter behavior. |
| src/Microsoft.TestPlatform.ObjectModel/Adapter/Interfaces/IFrameworkHandle.cs | Marks the property obsolete (error) at the public interface level. |
| src/Microsoft.TestPlatform.CrossPlatEngine/Execution/BaseRunTests.cs | Removes engine logic that read the property to affect run behavior. |
| src/Microsoft.TestPlatform.CrossPlatEngine/Adapter/FrameworkHandle.cs | Marks implementation property obsolete (error). |
| src/Microsoft.TestPlatform.Common/ExtensionDecorators/SerialTestRunDecoratorFrameworkHandle.cs | Marks decorator property obsolete (error) but still forwards to obsolete member. |
| src/Microsoft.TestPlatform.Common/ExtensionDecorators/SerialTestRunDecorator.cs | Marks decorator property obsolete (error) but still forwards to obsolete member. |
| /// and should be used only when absolutely required as using it degrades the performance of the subsequent run. | ||
| /// It throws InvalidOperationException when it is attempted to be enabled when keepAlive is false. | ||
| /// </summary> | ||
| [Obsolete("This property has no effect", error: true)] |
There was a problem hiding this comment.
Marking a member on a public interface as [Obsolete(..., error: true)] is an immediate compile-time breaking change for any external adapter that implements IFrameworkHandle or references the property. If the intent is “deprecate” rather than “remove now”, consider changing to error: false (warning) and planning removal in a major version, or introducing a new interface/versioned API so existing implementers aren’t forced into a hard break.
| [Obsolete("This property has no effect", error: true)] | |
| [Obsolete("This property has no effect", error: false)] |
| /// and should be used only when absolutely required as using it degrades the performance of the subsequent run. | ||
| /// It throws InvalidOperationException when it is attempted to be enabled when keepAlive is false. | ||
| /// </summary> | ||
| [Obsolete("This property has no effect", error: true)] | ||
| bool EnableShutdownAfterTestRun { get; set; } |
There was a problem hiding this comment.
The XML doc comment still describes functional behavior (“throws InvalidOperationException…”, perf impact) but the new obsolete message says the property has no effect. Please update the XML doc to match the new contract (no-op / deprecated) to avoid misleading adapter authors.
| } | ||
|
|
||
| [Obsolete("This property has no effect", error: true)] | ||
| public bool EnableShutdownAfterTestRun { get => _frameworkHandle.EnableShutdownAfterTestRun; set => _frameworkHandle.EnableShutdownAfterTestRun = value; } |
There was a problem hiding this comment.
This property forwards to _frameworkHandle.EnableShutdownAfterTestRun, which is now [Obsolete(..., error: true)] on the interface. Accessing an obsolete member with error: true typically fails compilation even inside another obsolete member, so this forwarding implementation is likely to break the build. Since the property is declared “no effect”, implement it as a local no-op (e.g., constant false + empty setter) to avoid referencing the obsolete member.
| public bool EnableShutdownAfterTestRun { get => _frameworkHandle.EnableShutdownAfterTestRun; set => _frameworkHandle.EnableShutdownAfterTestRun = value; } | |
| public bool EnableShutdownAfterTestRun { get => false; set { } } |
|
https://grep.app/search?q=EnableShutdownAfterTestRun looks to be used in places where I would expect it to be used, go ahead. |
|
probably with error:false, we will break the users when we remove it, and having one extra property that we don't use is not a problem. |
Co-authored-by: Youssef Victor <youssefvictor00@gmail.com>
| /// and should be used only when absolutely required as using it degrades the performance of the subsequent run. | ||
| /// It throws InvalidOperationException when it is attempted to be enabled when keepAlive is false. | ||
| /// </summary> | ||
| [Obsolete("This property has no effect", error: false)] | ||
| public bool EnableShutdownAfterTestRun { get; set; } |
There was a problem hiding this comment.
Same issue as on the interface: the implementation’s doc comment still claims it can throw and affects performance, but the property is now explicitly a no-op. Update/remove the outdated documentation to avoid misleading future maintainers and API consumers.
|
|
||
| public bool EnableShutdownAfterTestRun { get => _frameworkHandle.EnableShutdownAfterTestRun; set => _frameworkHandle.EnableShutdownAfterTestRun = value; } | ||
| [Obsolete("This property has no effect", error: false)] | ||
| public bool EnableShutdownAfterTestRun { get => _frameworkHandle.EnableShutdownAfterTestRun; set => _frameworkHandle.EnableShutdownAfterTestRun = value; } |
There was a problem hiding this comment.
Line 116 is mis-indented relative to surrounding members, which can violate formatting/style rules and may fail style analyzers in builds. Indent public bool EnableShutdownAfterTestRun ... to match the class’s standard member indentation.
| public bool EnableShutdownAfterTestRun { get => _frameworkHandle.EnableShutdownAfterTestRun; set => _frameworkHandle.EnableShutdownAfterTestRun = value; } | |
| public bool EnableShutdownAfterTestRun { get => _frameworkHandle.EnableShutdownAfterTestRun; set => _frameworkHandle.EnableShutdownAfterTestRun = value; } |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@nohwnd Thoughts?