Skip to content
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

Target frameworks not matching #229

Merged
merged 2 commits into from
Aug 3, 2020
Merged

Conversation

nohwnd
Copy link
Contributor

@nohwnd nohwnd commented Aug 3, 2020

Nuget.Frameworks started returning DotNetFrameworkName as "net5.0"
which is what vstest will pass down as the target framework name.

There is some guidance how to do that better, but this is a quick fix
to be able to run xUnit tests on net5.0-preview8 and newer, before we
figure out the best way to do this.

nohwnd added 2 commits August 3, 2020 10:28
Nuget.Frameworks started returning DotNetFrameworkName as "net5.0"
which is what vstest will pass down as the target framework name.

There is some guidance how to do that better, but this is a quick fix
to be able to run xUnit tests on net5.0-preview8 and newer, before we
figure out the best way to do this.
@nohwnd
Copy link
Contributor Author

nohwnd commented Aug 3, 2020

@bradwilson could you merge this please and release a new version so we can use it in dotnet templates? I know the fix is version specific and will break when we have net6.x, but that gives us a lot of time to fix this properly.

@clairernovotny
Copy link
Member

I'll merge and get this out as 2.4.3, but I'll hold you to a proper fix ;)

@clairernovotny clairernovotny merged commit 1df9085 into xunit:master Aug 3, 2020
@clairernovotny
Copy link
Member

2.4.3 is live with this fix https://www.nuget.org/packages/xunit.runner.visualstudio/2.4.3

@@ -113,7 +113,8 @@ public bool IsMatchingTargetFramework()

#if NETCOREAPP
return string.IsNullOrWhiteSpace(TargetFrameworkVersion) ||// Short circuit on null since we don't have anything to detect, return true
(TargetFrameworkVersion.StartsWith(".NETCoreApp,", StringComparison.OrdinalIgnoreCase) ||
(TargetFrameworkVersion.StartsWith("net5", StringComparison.OrdinalIgnoreCase) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about net6? We will soon branch to it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should have a follow up to fix this properly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nkolev92 what is the correct fix for this?

Copy link

@nkolev92 nkolev92 Aug 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copying my comment from the internal e-mail thread:

I think the guidance would be to never depend on the short name, or anything NuGet generated, but depend on the TFI/TFV.
If you ever have the NuGetFramework type, use the `Framework` property.

Let's not the same mistake as NuGet that caused: NuGet/Home#5154 :)

Copy link
Member

@clairernovotny clairernovotny Aug 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though to be fair, this value is not the TFM the user uses. It is the evaluated MSBuild property.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I'm not 100% familiar with the code here, but the general rule is to use props like TFI etc.

We probably have other places in the stack where this is being done, so we should do our best to weed them out.

Copy link
Contributor Author

@nohwnd nohwnd Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was responding to Viktor, the UI did not update for me: Then I need to fix this faster, or add another condition to this if, and to the two ifs that are in vstest that match runtime providers.

pranavkm added a commit to dotnet/arcade that referenced this pull request Aug 6, 2020
Using the 2.4.1 version of the runner fails with "Make sure that test discoverer & executors are registered and platform & framework version settings are appropriate and try again." when executing `dotnet test` on .NET 5 project. 2,4,3 was specifically released to address this. See xunit/visualstudio.xunit#229.
pranavkm added a commit to dotnet/arcade that referenced this pull request Aug 24, 2020
* Use Xunit runner that is compatible with .NET 5

Using the 2.4.1 version of the runner fails with "Make sure that test discoverer & executors are registered and platform & framework version settings are appropriate and try again." when executing `dotnet test` on .NET 5 project. 2,4,3 was specifically released to address this. See xunit/visualstudio.xunit#229.
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.

4 participants