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.

Sign up for GitHub

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

Target frameworks not matching #229

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
15ab67e
Target frameworks not matching
nohwnd Aug 3, 2020
a276545
Revert changes
nohwnd Aug 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/xunit.runner.visualstudio/Utility/RunSettings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

TargetFrameworkVersion.StartsWith(".NETCoreApp,", StringComparison.OrdinalIgnoreCase) ||
TargetFrameworkVersion.StartsWith("FrameworkCore10", StringComparison.OrdinalIgnoreCase));
#elif WINDOWS_UAP
return string.IsNullOrWhiteSpace(TargetFrameworkVersion) || // Short circuit on null since we don't have anything to detect, return true
Expand Down