Skip to content

Conversation

@bergmeister
Copy link
Contributor

@bergmeister bergmeister commented Jul 9, 2018

PR Summary

image

PR Checklist

@bergmeister bergmeister changed the title Add xunit project to PowerShell.sln and make it runable from within VisualStudio WIP Add xunit project to PowerShell.sln and make it runable from within VisualStudio Jul 9, 2018
@bergmeister bergmeister changed the title WIP Add xunit project to PowerShell.sln and make it runable from within VisualStudio Add xunit project to PowerShell.sln and make it runable from within VisualStudio Jul 9, 2018
@bergmeister bergmeister changed the title Add xunit project to PowerShell.sln and make it runable from within VisualStudio WIP Add xunit project to PowerShell.sln and make it runable from within VisualStudio Jul 10, 2018
@bergmeister bergmeister changed the title WIP Add xunit project to PowerShell.sln and make it runable from within VisualStudio Add xunit project to PowerShell.sln and make it runable from within VisualStudio Jul 10, 2018
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

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?

Copy link
Contributor Author

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...

@adityapatwardhan
Copy link
Member

@bergmeister Please have a look at merge conflicts.

@bergmeister
Copy link
Contributor Author

bergmeister commented Jul 13, 2018

@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.
EDIT: after some debugging/experimenting, it seems it was not the referenced PR but some other recently merged PR that breaks my branch in the sense that the XUnit project now does not load in VS any more.... The configuration changes in the references PR to the sln file should be still be re-reviewed though as they seem to be accidental.

{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
Copy link
Member

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?

Copy link
Collaborator

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.

Copy link
Contributor Author

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

Copy link
Member

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.

image

Copy link
Contributor Author

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 07BFD271 lines
  • Revert the removal of the 2 8F63D134 lines
  • Reorder lines so that VS does not automatically re-order them on save,

@bergmeister
Copy link
Contributor Author

bergmeister commented Jul 17, 2018

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?

@daxian-dbw
Copy link
Member

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 😄

@bergmeister bergmeister requested a review from dantraMSFT as a code owner July 23, 2018 21:47
@anmenaga
Copy link

@adityapatwardhan This looks ready for merge.

@adityapatwardhan adityapatwardhan merged commit 5717a5d into PowerShell:master Aug 2, 2018
@adityapatwardhan
Copy link
Member

@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" />
Copy link
Collaborator

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 :-)

Copy link
Contributor Author

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

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

@powercode
Copy link
Collaborator

@bergmeister Really happy about this PR! Thx!

@bergmeister
Copy link
Contributor Author

Thanks @powercode , I also plan to do something similar for the hosting tests. Thanks for your efforts about the ReSharper settings as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

XUnit test project not part of Visual Studio solution PowerShell.sln -> make XUnit runable from within VS test explorer

8 participants