-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Add xunit project to PowerShell.sln and make it runable from within VisualStudio #7254
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
Conversation
…to run it within vs
…o AddXUnitToVs
PowerShell.sln
Outdated
| {73EA0BE6-C0C5-4B56-A5AA-DADA4C01D690}.Debug|Any CPU.Build.0 = Linux|Any CPU | ||
| {73EA0BE6-C0C5-4B56-A5AA-DADA4C01D690}.CodeCoverage|Any CPU.ActiveCfg = Linux|Any CPU | ||
| {73EA0BE6-C0C5-4B56-A5AA-DADA4C01D690}.CodeCoverage|Any CPU.Build.0 = Linux|Any CPU | ||
| {190BB016-1395-4944-853F-98F8A4277483}.CodeCoverage|Any CPU.ActiveCfg = Release|Any CPU |
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.
Can you please explain why these changes to ConfigurationPlatforms are necessary?
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.
This is done automatically by VS when adding an existing project...
|
@bergmeister Please have a look at merge conflicts. |
|
@adityapatwardhan I cannot take the upstream changes because PR #6926 invalidated the PowerShell solution file in such a way that it even prevents the XUnit project now from loading. I will therefore wait until the solution file has been fixed in master. |
…ToVs # Conflicts: # PowerShell.sln
…o AddXUnitToVs
…lstudio package is now sufficient for running tests in VS in the latest version (15.7.5)
…ease version (was already pre-release)
| {07BFD271-8992-4F34-9091-6CFC3E224A24}.Release|Any CPU.ActiveCfg = Release|Any CPU | ||
| {07BFD271-8992-4F34-9091-6CFC3E224A24}.Release|Any CPU.Build.0 = Release|Any CPU | ||
| {8F63D134-E413-4181-936D-D82F3F5F1D85}.CodeCoverage|Any CPU.ActiveCfg = CodeCoverage|Any CPU | ||
| {8F63D134-E413-4181-936D-D82F3F5F1D85}.CodeCoverage|Any CPU.Build.0 = CodeCoverage|Any CPU |
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.
You removed these two entries that are associated with 8F63D134-E413-4181-936D-D82F3F5F1D85, but left two entries associated with 07BFD271-8992-4F34-9091-6CFC3E224A24 above. Is that intentional?
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.
I believe it is better to rebase then try to resolve conflicts in such auto changed file.
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.
The conflict is already resolved and I resolved it by taking HEAD and adding the project again to the solution
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.
@bergmeister Please take a look at the highlighted entries in the screenshot below. It looks to me you shouldn't remove the two 8F63D134 entries.
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.
@daxian-dbw Sorry, thanks for reminding me to address your comment as well. Yes, I made 3 commits to address this:
- Remove the 2 redundant
07BFD271lines - Revert the removal of the 2
8F63D134lines - Reorder lines so that VS does not automatically re-order them on save,
|
UPDATE: I addressed the comments about the solution file and also updated the XUnit Nuget package from the preview version of 2.4.0 to the RTM version as it got published today. Shall I also update the xunit reference in test/hosting/hosting.tests.csproj from 2.3.1 to 2.4.0? |
I'm fine with updating the xunit reference, as long as it doesn't break our xunit test run 😄 |
|
@adityapatwardhan This looks ready for merge. |
|
@bergmeister Thank you for your contribution! |
| <PackageReference Include="xunit" Version="2.3.1" /> | ||
| <PackageReference Include="xunit" Version="2.4.0" /> | ||
| <!-- DotNetCliToolReference element specifies the CLI tool that the user wants to restore in the context of the project. --> | ||
| <DotNetCliToolReference Include="dotnet-xunit" Version="2.3.1" /> |
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.
@bergmeister We skipped the old version :-)
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 you mean dotnet-xunit? Version 2.4.0 of it is still in preview
https://www.nuget.org/packages/dotnet-xunit/2.4.0-beta.1.build3958
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.
Yes, we add it in csharp.tests.csproj in the PR.
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.
Ok, I see what you mean now. If it's not essential, then I'd still just wait for 2.4.0 of this package to become RTM.
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.
We can wait. My comment is informational only.
|
@bergmeister Really happy about this PR! Thx! |
|
Thanks @powercode , I also plan to do something similar for the hosting tests. Thanks for your efforts about the ReSharper settings as well. |

PR Summary
PowerShell.slnand tweaking it to make the project load in VS (by adding theToolsVersion) and making it runable in the VS test explorer by adding the required NuGet packagesxunit.runner.visualstudio.xunitpackage from2.4.0-beta.2.build4010to2.4.0(RTM) that got recently releasedPR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests