Skip to content

.NET 7.0 support #4224

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

Jump to bottom
Merged
merged 14 commits into from
Nov 14, 2022
Merged

.NET 7.0 support #4224

merged 14 commits into from
Nov 14, 2022

Conversation

KonH
Copy link
Contributor

@KonH KonH commented Oct 8, 2022

Changes required to support .NET 7.0
Work in progress, .NET 7.0 RC should be changed when a stable version is released
Related (and more details provided) to #4170

KonH added 9 commits October 8, 2022 22:43
stevenaw
stevenaw reviewed Oct 24, 2022
View reviewed changes
nuget/nunitlite/nunitlite.nuspec
@@ -46,6 +46,9 @@ How to use this package:
<group targetFramework="net6.0">
<dependency id="NUnit" version="[$version$]" />
</group>
<group targetFramework="net7.0">
Copy link
Member

Choose a reason for hiding this comment

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

@manfred-brands Wondering if I can ask your confirmation of my understanding here since you did the previous upgrades in this area. This would apply to the framework of the consuming app I think, right?

Copy link
Member

Choose a reason for hiding this comment

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

@stevenaw If the app consuming nunitlite targets any of the listed framework, it will automatically add a transient dependency on the packages listed here.

@stevenaw
Copy link
Member

Thanks for your hard work on working through some of the issues on this @KonH . As you mention, this won't be able to be merged until after Hacktoberfestt is complete so I've added the hacktoberfest-accepted label as requested 🙂
I've asked for a bit of help confirming the changes given we have some other team members with more previous experience in this area of the codebase, though the changeset certainly appears consistent with previous net5/6 upgrade PRs.

manfred-brands
manfred-brands reviewed Oct 24, 2022
View reviewed changes
src/NUnitFramework/tests/Assertions/WarningTests.cs
Comment on lines +226 to +229
#if NET7_0_OR_GREATER
[TestCase(nameof(WarningFixture.WarningInThreadPoolQueueUserWorkItem), 4, ExcludePlatform = "MacOSX")]
[TestCase(nameof(WarningFixture.WarningInThreadPoolQueueUserWorkItem), 5, IncludePlatform = "MacOSX")]
#else
Copy link
Member

Choose a reason for hiding this comment

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

Is this because of "InvokeStub_" lines not removed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not exactly, I described it here - #4170 (comment)

manfred-brands
manfred-brands reviewed Oct 24, 2022
View reviewed changes
Copy link
Member

@manfred-brands manfred-brands left a comment

Choose a reason for hiding this comment

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

Some small suggestions, otherwise it looks good to me.

@KonH
Copy link
Contributor Author

KonH commented Nov 8, 2022

.NET 7 was released, and final changes applied

@KonH KonH marked this pull request as ready for review November 8, 2022 21:00
@KonH
Copy link
Contributor Author

KonH commented Nov 8, 2022

The latest fail looks flaky, the previous run completed successfully and changes between them are not related.
The log shows this line in failed run:

1) Cancelled : NUnit.Framework.Constraints.CollectionEquivalentConstraintTests.LargeStringCollection
Test cancelled by user
  at NUnit.Framework.Internal.Commands.DelegatingTestCommand.RunTestMethodInThreadAbortSafeZone (NUnit.Framework.Internal.TestExecutionContext context, System.Action action) [0x0002f] in <8a6beb3f2b6c4711aa41176b631ad128>:0 
  at NUnit.Framework.Internal.Commands.BeforeAndAfterTestCommand.Execute (NUnit.Framework.Internal.TestExecutionContext context) [0x00060] in <8a6beb3f2b6c4711aa41176b631ad128>:0 
  at NUnit.Framework.Internal.Execution.SimpleWorkItem+<>c__DisplayClass3_0.<PerformWork>b__0 () [0x00011] in <8a6beb3f2b6c4711aa41176b631ad128>:0 
  at NUnit.Framework.Internal.ContextUtils+<>c__DisplayClass1_0`1[T].<DoIsolated>b__0 (System.Object _) [0x00000] in <8a6beb3f2b6c4711aa41176b631ad128>:0 
  at System.Threading.ExecutionContext.RunInternal (System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, System.Object state, System.Boolean preserveSyncCtx) [0x00071] in <bab7d1a00376483b944db50cdc31e41d>:0 
  at System.Threading.ExecutionContext.Run (System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, System.Object state, System.Boolean preserveSyncCtx) [0x00000] in <bab7d1a00376483b944db50cdc31e41d>:0 
  at System.Threading.ExecutionContext.Run (System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, System.Object state) [0x0002b] in <bab7d1a00376483b944db50cdc31e41d>:0 
  at NUnit.Framework.Internal.ContextUtils.DoIsolated (System.Threading.ContextCallback callback, System.Object state) [0x00025] in <8a6beb3f2b6c4711aa41176b631ad128>:0 
  at NUnit.Framework.Internal.ContextUtils.DoIsolated[T] (System.Func`1[TResult] func) [0x0001a] in <8a6beb3f2b6c4711aa41176b631ad128>:0 
  at NUnit.Framework.Internal.Execution.SimpleWorkItem.PerformWork () [0x0001b] in <8a6beb3f2b6c4711aa41176b631ad128>:0

@stevenaw
Copy link
Member

Thanks @KonH I've pulled the changes and they run + pass fine locally for me.
As far as the failing test, you are right this one has had a history of occasionally exceeding the timeout in CI.

@nunit/framework-team This failing test was also a confusing issue for contributors during Hacktoberfest. Are there any objections to @KonH temporarily bumping up this value until it can be investigated? Perhaps just the one LargeStringCollection() test here can have the "FAIL_TIME" multiplied by 2: `https://github.com/nunit/nunit/blob/master/src/NUnitFramework/tests/Constraints/CollectionEquivalentConstraintTests.cs#L368 as the generic versions still seem under the threshold

@KonH
Copy link
Contributor Author

KonH commented Nov 12, 2022

@stevenaw I have increased timeout, now tests passed

@stevenaw stevenaw mentioned this pull request Nov 14, 2022
Open
stevenaw
stevenaw approved these changes Nov 14, 2022
View reviewed changes
Copy link
Member

@stevenaw stevenaw left a comment

Choose a reason for hiding this comment

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

LGTM thanks for your contribution @KonH

@stevenaw stevenaw merged commit 14f2d35 into nunit:master Nov 14, 2022
stevenaw pushed a commit that referenced this pull request Feb 26, 2023
@KonH @stevenaw
.NET 7.0 support (#4224)
350cae7 
* Initial changes to support .NET 7 RC - #4170

Mostly based on 6cbe01e

* NuGet packaging changes to support .NET 7 RC - #4170

Based on 6cbe01e

* Continuous integration changes to support .NET 7 RC - #4170

Based on 6cbe01e

* Try to fix .NET 7.0 lookup on Azure Pipelines - #4170

* It looks like we need both .NET 6.0 and .NET 7.0 to run tests on Azure Pipelines - #4170

* Use valid version for .NET 6.0 on Azure Pipelines - #4170

* Use different test settings for .NET 7 to cover corner-case - #4170

* Add potential workaround for flaky test behaviour on .NET 7 - #4170

* On MacOS test behaviour is different on .NET 7 - #4170

* Change setup-dotnet action version (for new lines only)

#4224 (comment)

* Use release version of .NET 7.0

* Refactor: do not copy collections, use single method

* Temporary increase test timeout

* Temporary increase test timeout #2
stevenaw pushed a commit that referenced this pull request Feb 26, 2023
@KonH @stevenaw
.NET 7.0 support (#4224)
d1565a8 
* Initial changes to support .NET 7 RC - #4170

Mostly based on 6cbe01e

* NuGet packaging changes to support .NET 7 RC - #4170

Based on 6cbe01e

* Continuous integration changes to support .NET 7 RC - #4170

Based on 6cbe01e

* Try to fix .NET 7.0 lookup on Azure Pipelines - #4170

* It looks like we need both .NET 6.0 and .NET 7.0 to run tests on Azure Pipelines - #4170

* Use valid version for .NET 6.0 on Azure Pipelines - #4170

* Use different test settings for .NET 7 to cover corner-case - #4170

* Add potential workaround for flaky test behaviour on .NET 7 - #4170

* On MacOS test behaviour is different on .NET 7 - #4170

* Change setup-dotnet action version (for new lines only)

#4224 (comment)

* Use release version of .NET 7.0

* Refactor: do not copy collections, use single method

* Temporary increase test timeout

* Temporary increase test timeout #2
@stevenaw stevenaw mentioned this pull request Feb 26, 2023
Merged
@stevenaw stevenaw mentioned this pull request Oct 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manfred-brands manfred-brands manfred-brands left review comments

@stevenaw stevenaw stevenaw approved these changes

Assignees
No one assigned
Labels
hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KonH @stevenaw @manfred-brands