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
Backport Issue 3984 (.NET6 support++) to v3.13 branch #4077
Conversation
MacOS test fails with a timeout, the test The existing timeouts are defined like: // The following tests are each running in 14ms to 46ms on my machine. Based on that,
// warn at 100ms and fail at 500ms
const int LARGE_COLLECTION_WARN_TIME = 100;
const int LARGE_COLLECTION_FAIL_TIME = 500; Changing this seems outside scope. |
@manfred-brands I recall that being a bit of a finnicky test which anecdotally appears to fail sometimes if the cloud VMs are under high utilization. |
</PropertyGroup> | ||
|
||
<ItemGroup> | ||
<PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies" Version="1.0.0" /> | ||
<PackageReference Include="jnm2.ReferenceAssemblies.net35" Version="1.0.1" /> | ||
</ItemGroup> | ||
|
||
<ItemGroup Condition="'$(TargetFramework)' == 'net40'"> | ||
<ItemGroup Condition="'$(TargetFramework)' == 'net40' OR '$(TargetFramework)' == 'net45'"> |
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'm not familiar with this change, but I'm assuming it fixes something :)
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.
It fixes consistency. nunit.framework.csproj was compiled with net35;net40;net45.
Other assemblies had various other version. They are now all aligned. The condition includes some Microsoft.Bcl packages for functionality that was added to the framework in 4.6
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.
Thanks @manfred-brands this looks good to me!
Most of my review focused on comparing branches and PRs. I'd appreciate a second pair of eyes on this
I'll be the second pair of eyes :). I'll take a look at it this weekend (hopefully today). |
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.
Also looks good to me and a nice simplification in multiple files.
I agree with @stevenaw comments and have some additional whitespace nit-pick.
{ | ||
var task = Task("TestNetStandard20 on " + runtime) | ||
.Description("Tests the .NET Standard 2.0 version of the framework on " + runtime) | ||
.WithCriteria(IsRunningOnWindows() || runtime != "net5.0-windows") | ||
.WithCriteria(IsRunningOnWindows() || !runtime.EndsWith("windows")) |
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 no longer have any entries in NetCoreTests
that ends with windows
.
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 the same as in main branch. It protects against someone updating NUnitRuntimeFrameworks
in Directory.Build.props
. We could remove the whole condition in both branches.
I tried to see if I could retrigger the build in azure devops, but the build is no longer there, and the branch does not exists in the nunit project, and the commit didn't work, so I'm not sure I can retrigger it. |
2b34128
to
f617fa0
Compare
I think I have addressed all issues. |
Upgrade to .NET 6.0.100 SDK Use dotnet core build also on Windows Unify frameworks
Rebased, is there anything holding this up being merged? |
Thanks @manfred-brands ! Looks like it built. LGTM 🎉 |
Fixes #4046
Upgrade to .NET 6.0.100 SDK
Use dotnet core build also on Windows
Unify frameworks