-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dotnet test improvements for MTP #47395
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
The purpose of this PR is to improve the handling of test project detection for MTP by correctly considering projects that set IsTestingPlatformApplication without setting IsTestProject, as well as to provide a clearer error message when MTP and VSTest are used together.
- Updated method signatures and type names from Module to TestModule.
- Modified test application initialization logic to enforce stricter validation of test projects.
- Removed redundant error messaging from TestingPlatformCommand in favor of improved error handling elsewhere.
Reviewed Changes
File | Description |
---|---|
src/Cli/dotnet/commands/dotnet-test/MSBuildHandler.cs | Updated type usage and added stricter invariant for test projects. |
src/Cli/dotnet/commands/dotnet-test/SolutionAndProjectUtility.cs | Updated project property retrieval logic for test modules. |
src/Cli/dotnet/commands/dotnet-test/Models.cs | Renamed record type from Module to TestModule. |
src/Cli/dotnet/commands/dotnet-test/TestingPlatformCommand.cs | Removed redundant error message in favor of new error handling. |
src/Cli/dotnet/commands/dotnet-test/MSBuildUtility.cs | Updated type usage from Module to TestModule consistently. |
src/Cli/dotnet/commands/dotnet-test/TestApplication.cs | Updated type usage and property exposure from Module to TestModule. |
Copilot reviewed 47 out of 47 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
src/Cli/dotnet/commands/dotnet-test/MSBuildHandler.cs:96
- Verify that the updated condition properly reflects the intended invariant so that only valid test projects progress to initialization; otherwise, unexpected exceptions may be thrown.
if (!module.IsTestProject && !module.IsTestingPlatformApplication)
src/Cli/dotnet/commands/dotnet-test/SolutionAndProjectUtility.cs:172
- Ensure that combining the conditions for IsTestProject and IsTestingPlatformApplication accurately excludes non-test projects without unintentionally filtering out valid configurations.
if (!isTestProject && !isTestingPlatformApplication)
src/Cli/dotnet/commands/dotnet-test/TestingPlatformCommand.cs:61
- Confirm that removing this error message does not leave a gap in user feedback for unsupported test application configurations; ensure that alternative messaging sufficiently informs the user.
_output.WriteMessage(LocalizableStrings.CmdUnsupportedVSTestTestApplicationsDescription, new SystemConsoleColor { ConsoleColor = ConsoleColor.Red });
42ab7b2
to
a7e8c22
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving but please wait for @mariam-abdulla green light!
Merging as discussed offline. @mariam-abdulla will create a follow-up PR to address the review comments. |
The handling of
IsTestProject
andIsTestingPlatformApplication
was not correct.Projects that were setting
IsTestingPlatformApplication
to true but not settingIsTestProject
were not considered, which is wrong.Improve error message for having MTP and VSTest together when using the new experience.
Rename
Module
toTestModule
for clarityFixes microsoft/testfx#5198
Fixes microsoft/testfx#5196