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

Jump to bottom

Migrate from NUnit 3 to NUnit 4 #783

Merged
merged 2 commits into from May 1, 2024
Merged

Migrate from NUnit 3 to NUnit 4 #783

merged 2 commits into from May 1, 2024

Conversation

Romfos
Copy link
Contributor

@Romfos Romfos commented Mar 4, 2024 •
edited

Changes:

  • Migrate to NUnit4
  • Update nuget packages for test projects

note: no changes in product, no need to release new nuget package

@Romfos Romfos changed the title Update to nunit4 Migrate from NUnit 3 to NUnit 4 Mar 4, 2024
alexandrnikitin
alexandrnikitin approved these changes Mar 4, 2024
View reviewed changes
Copy link
Member

@alexandrnikitin alexandrnikitin left a comment

Choose a reason for hiding this comment

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

LGTM!

Thank you! @Romfos
I'm sorry, I stopped following .NET development. What is the general recommendation regarding NUnit.Framework.Legacy and ClassicAssert? Do we need to migrate them to the newer API?

tests/NSubstitute.Benchmarks/NSubstitute.Benchmarks.csproj
@@ -6,7 +6,7 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="BenchmarkDotNet" Version="0.13.10" />
<PackageReference Include="BenchmarkDotNet" Version="0.13.12" />
Copy link
Member

Choose a reason for hiding this comment

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

looks unrelated to NUnit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this related to Update nuget packages for test projects

tests/NSubstitute.Acceptance.Specs/ReceivedCalls.cs
@@ -1,6 +1,7 @@
using NSubstitute.Exceptions;
using NSubstitute.ReceivedExtensions;
using NUnit.Framework;
using NUnit.Framework.Legacy;
Copy link
Member

Choose a reason for hiding this comment

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

unused?

Copy link
Contributor Author

@Romfos Romfos Mar 4, 2024 •
edited

Choose a reason for hiding this comment

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

this is needed for StringAssert

@alexandrnikitin
Copy link
Member

alexandrnikitin commented Mar 4, 2024 •
edited

It looks like TestCodeFromDocs project needs to be updated too.

NSubstitute/build/build.fs

Lines 164 to 166 in e67780b

<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.8.0" />
<PackageReference Include="NUnit" Version="3.14.0" />
<PackageReference Include="NUnit3TestAdapter" Version="4.5.0" />

@Romfos
Copy link
Contributor Author

Romfos commented Mar 4, 2024

It looks like TestCodeFromDocs project needs to be updated too.

NSubstitute/build/build.fs

Lines 164 to 166 in e67780b

<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.8.0" />
<PackageReference Include="NUnit" Version="3.14.0" />
<PackageReference Include="NUnit3TestAdapter" Version="4.5.0" />

need to be checked after #776 merge

@Romfos
Copy link
Contributor Author

Romfos commented Mar 4, 2024

LGTM!

Thank you! @Romfos I'm sorry, I stopped following .NET development. What is the general recommendation regarding NUnit.Framework.Legacy and ClassicAssert? Do we need to migrate them to the newer API?

I was not able to find recommendations for now, but this looks like temporary solution for migration from v3 to v4.
I think that in long term it should be refactored

@dtchepak
Copy link
Member

LGTM!

Thank you! @Romfos I'm sorry, I stopped following .NET development. What is the general recommendation regarding NUnit.Framework.Legacy and ClassicAssert? Do we need to migrate them to the newer API?

Seems should replace with Assert.That.

@304NotModified
Copy link
Contributor

LGTM!
Thank you! @Romfos I'm sorry, I stopped following .NET development. What is the general recommendation regarding NUnit.Framework.Legacy and ClassicAssert? Do we need to migrate them to the newer API?

Seems should replace with Assert.That.

Indeed, but IMO that could be another PR and this one could be merged

@dtchepak dtchepak merged commit ef7a2d0 into nsubstitute:main May 1, 2024
12 checks passed
@dtchepak
Copy link
Member

dtchepak commented May 1, 2024

Thanks for this @Romfos ! And thanks for the reviews @alexandrnikitin and @304NotModified 🙇

@Romfos Romfos deleted the nunit4 branch May 1, 2024 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexandrnikitin alexandrnikitin alexandrnikitin approved these changes

@304NotModified 304NotModified 304NotModified approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Romfos @alexandrnikitin @dtchepak @304NotModified