Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes redundant null checks in several components across Microsoft.Testing.Platform, the VSTest bridge, the MSTest adapter platform services, and an analyzer code fix—aligning the code with existing nullability contracts and non-null field initializations.
Changes:
- Removed dead null checks on fields that are always initialized (e.g.,
ConcurrentQueue,_propertiesdictionary). - Simplified conditional logic by relying on
[NotNullWhen(true)]out-parameter contracts for command-line parsing. - Simplified Roslyn code-fix logic by removing a redundant
ReturnType != nullcheck.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Platform/Microsoft.Testing.Platform/Telemetry/TelemetryManager.cs | Treats GetFolderPath(...) result as non-null and removes redundant guarding before Path.Combine. |
| src/Platform/Microsoft.Testing.Platform/Services/StopPoliciesService.cs | Removes redundant _maxFailedTestsCallbacks is null guard (queue is always initialized). |
| src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/ContextAdapterBase.cs | Removes redundant filterExpressions is not null check, relying on the TryGetOptionArgumentList nullability contract. |
| src/Analyzers/MSTest.Analyzers.CodeFixes/TestMethodShouldBeValidCodeFix.cs | Removes redundant null check for MethodDeclarationSyntax.ReturnType. |
| src/Adapter/MSTestAdapter.PlatformServices/Services/TestContextImplementation.cs | Removes redundant _properties == null guard (_properties is always constructed in the ctor). |
| src/Adapter/MSTestAdapter.PlatformServices/AssemblyResolver.cs | Removes redundant null guard for a non-nullable List<RecursiveDirectoryPath> parameter. |
|
|
||
| ITestApplicationModuleInfo testApplicationModuleInfo = serviceProvider.GetTestApplicationModuleInfo(); | ||
| #pragma warning disable RS0030 // Do not use banned APIs - There is no easy way to disable it for all members | ||
| string? directory = environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData, Environment.SpecialFolderOption.Create); |
There was a problem hiding this comment.
This one I'm not sure about. Some tests might be mocking stuff in a way that breaks this. If we can write the tests in a better way, that could be a good option. Otherwise, we probably keep this one for now.
But other than that, the rest of the changes look very good!
| return false; | ||
| } | ||
|
|
||
| return _properties.TryGetValue(propertyName, out propertyValue); |
There was a problem hiding this comment.
error IDE0022: (NETCORE_ENGINEERING_TELEMETRY=Build) Use expression body for method
No description provided.