-
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
Implement dotnet test help option #41142
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.
Did review round some question and cleanup
src/Cli/dotnet/commands/dotnet-test/IPC/Serializers/VoidResponseSerializer.cs
Outdated
Show resolved
Hide resolved
...orts/Current/Microsoft.Common.targets/ImportAfter/Microsoft.TestPlatform.ImportAfter.targets
Show resolved
Hide resolved
This reverts commit 1c38cc1.
@dotnet/domestic-cat We are using $(MicrosoftNETBuildTasksAssembly) in Microsoft.TestPlatform.ImportAfter.targets |
That seems to be defined in Microsoft.NET.Sdk.Common.targets, which only comes in if you either import it directly or reference it indirectly by, for instance, using Sdk="Microsoft.NET.Sdk". I looked at one of the failing tests, and it was not an SDK-style project, and it didn't import that .targets file directly, which explains why it evaluated to "". You should be able to change the test asset to import it (and make sure you import it before Microsoft.TestPlatform.ImportAfter.targets). Do you need this task in legacy .*proj files? |
When you say legacy, do you mean non SDK-style projects? |
Yes |
Then, we don't need the task there. |
Perfect, so we would just need to fix up the tests to make sure we only need to have that property defined in sdk-style test projects. Let me know if you need any help with that! |
Yes, please let me know what should be done in that case. |
…rosoft.TestPlatform.ImportAfter.targets
…b.com/dotnet/sdk into dev/mabdullah/ImplementDotnetTestHelp
…b.com/dotnet/sdk into dev/mabdullah/ImplementDotnetTestHelp
/azp run |
Azure Pipelines successfully started running 4 pipeline(s). |
@Forgind do we have some ETA to merge this first part of @mariam-abdulla PR? We need it to move to the next and unblock the task. |
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.
LGTM and tests are green now @Forgind @baronfel are you good for the merge, can you review?
For now we went for "manual" resolution of Microsoft.NET.Build.Tasks.dll
https://github.com/dotnet/sdk/pull/41142/files#diff-3f7b2597025eca7c8bdaa8632fab1a3a6894415a704c2b08c2b0760ac708ba39 to fix the test failure
And added the cross targeting part close to the other one https://github.com/dotnet/sdk/pull/41142/files#diff-177f9060fda1f47c96d4e08705bbaab76098d76df09f8911eeea06586db65fc6
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.
LGTM - the main thing was the Task location and that's been addressed. Thanks folks, excited to see how this evolves.
@MarcoRossignoli / @mariam-abdulla please feel free to merge whenever you like - consider squashing though since many of the individual commits in this PR aren't meaningful to the end result. |
Relates to #45927 |
No description provided.