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

Update Akka.TestKit.NUnit to NUnit v4 (3rd attempt) #136

Merged
merged 14 commits into from
Sep 6, 2024

Conversation

milang
Copy link
Contributor

@milang milang commented Sep 4, 2024

Fixes #111.

Changes

This PR extends pull request #131 by @UrsMetz, which in turn builds on the work of @SeanKilleen in #112.

Improvements in NUnit TestKit adapters

  • ⚠️ Akka.TestKit.NUnit
    • Uses the latest NUnit 4 (4.2.2)
    • Targets net6.0 and net462, just like NUnit 4
  • ⚠️ Akka.TestKit.NUnit3
    • Uses the latest NUnit 3 (3.14.0)
    • Targets netstandard2.0, just like NUnit 3
      • NUnit 3 actually supports net35;net40;net45;netstandard2.0, but we have to exclude frameworks not supported by [email protected]
  • Implementations of NUnit 4 and NUnit 3 adapters are now almost identical, both using NUnit's constraint model. The only difference is how the assertion message is constructed, because NUnit 4 removed support for format and params specification.
  • ITestKitAssertions.AssertEqual(..., comparer, ...) now uses NUnit constraint model instead of custom assertion implementation. This makes failure reports more consistent with the rest of NUnit output.
  • All dependencies have been bumped to the latest version. Merging this PR should allow all existing dependabot pull requests to be closed (except Nuke.Common).
    dotnet-outdated

Improvements in NuGet packages

  • Icon and license are now specified by PackageIcon and PackageLicenseExpression properties, eliminating warnings during dotnet pack. Akka icon with dimensions 128x128px is now embedded in .nupkg file, as recommended by NuGet.
  • Symbol files (.pdb) are now included directly in the .nupkg files. This only increases the final .nupkg size by ~20 kB and removes the need to publish separate .snupkg file to a symbol server. See Microsoft guidance.
  • Current year is automatically set in copyright message

The following screenshot compares currently published .nupkg file (version 1.5.24) with .nupkg as proposed by this pull request. Please note that the green "source link", "deterministic exe/dll", and "compiler flags" shields typically require the package to be built with the latest .NET SDK (mine was built using .NET SDK 8.0.401). However this repository's GitHub Actions are currently configured to use .NET SDK 6.

Akka.TestKit.NUnit.nupkg comparison

Checklist

For significant changes, please ensure that the following have been completed (delete if not relevant):

UrsMetz and others added 11 commits August 17, 2024 18:27
This commit is based on the work of Sean Killeen
in PR akkadotnet#112.
Additional changes:
* add net462 as target framework
* set C# version in test projects to C# 10 so that file scoped namespaces work
- This improves consistency between NUnit 3 and 4 adapters
Our adapters are no longer using custom assertions. This will result in
failure reports that are consistent with other NUnit reports, especially
when the report output changes (like it did between NUnit 3 and 4).
This commit only changes indentation whitespace, making this MSBuild
file consistent with other MSBuild files in the repository (.csproj).
RELEASE_NOTES.md Outdated Show resolved Hide resolved
@milang
Copy link
Contributor Author

milang commented Sep 6, 2024

@FarrisPandell addressed your feedback, ready for next round.

Akka.TestKit 1.5.28, Microsoft.NET.Test.Sdk 17.11.1
@milang
Copy link
Contributor Author

milang commented Sep 6, 2024

@Aaronontheweb any feedback for this pull request? I updated the Akka.TestKit dependency to 1.5.28, new Akka.TestKit.NUnit release would nicely match yesterday's Akka.NET release?

@Aaronontheweb
Copy link
Member

@milang @FarrisPandell my apologies - I've had GitHub Notifications turned off for 10 years (unless I get explicitly mentioned), so this is the first I'm seeing this. CI/CD is failing - any idea what that's about?

@Aaronontheweb
Copy link
Member

Overall the NuGet package structure you've proposed looks good to me

@milang
Copy link
Contributor Author

milang commented Sep 6, 2024

@Aaronontheweb

CI/CD is failing - any idea what that's about?

Nuke is complaining that "Release Notes should not be empty". I tracked it down to newlines that I added after version headers in RELEASE_NOTES.md - the newlines were recommended by markdownlint, but they were confusing Nuke build.

I pushed a new commit that removes the empty newlines, hopefully the build will pass now.

@Aaronontheweb
Copy link
Member

@Aaronontheweb

CI/CD is failing - any idea what that's about?

Nuke is complaining that "Release Notes should not be empty". I tracked it down to newlines that I added after version headers in RELEASE_NOTES.md - the newlines were recommended by markdownlint, but they were confusing Nuke build.

I pushed a new commit that removes the empty newlines, hopefully the build will pass now.

Ugh, yes NUKE is very opinionated about this. Ran into that myself before.

Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

LGTM

@Aaronontheweb Aaronontheweb enabled auto-merge (squash) September 6, 2024 20:26
@Aaronontheweb Aaronontheweb merged commit 42348b1 into akkadotnet:dev Sep 6, 2024
2 checks passed
@Aaronontheweb
Copy link
Member

So it's release as v1.6.0-preview1 on NuGet - let me know if that's acceptable

@Aaronontheweb
Copy link
Member

I'm also fine doing it as v1.5.* package if there's buy-in

@UrsMetz
Copy link
Contributor

UrsMetz commented Sep 7, 2024

@Aaronontheweb would a release as 1.6.* mean that a corresponding Akka 1.6.* was needed to use the NUnit v4 compatible version of Akka.TestKit.NUnit?
I think currently all Akka packages tend to share the same minor version.
My team is eagerly waiting for the NUnit 4 compatibility.

@SeanKilleen
Copy link

@milang thanks for getting this across the finish line! Awesome work 🎉 👏

@milang
Copy link
Contributor Author

milang commented Sep 9, 2024

@Aaronontheweb

So it's release as v1.6.0-preview1 on NuGet - let me know if that's acceptable
I'm also fine doing it as v1.5.* package if there's buy-in

@UrsMetz

Would a release as 1.6.* mean that a corresponding Akka 1.6.* was needed to use the NUnit v4 compatible version of Akka.TestKit.NUnit? I think currently all Akka packages tend to share the same minor version.

Sorry, I didn't realize that the major/minor version was fixed at 1.5 to match rest of Akka.NET. Initially I bumped to 1.6 to follow semantic versioning, indicating that major version of NUnit dependency changed. That being said, I have no issue with staying on 1.5, and in light of Urs' comment that would be the better approach.

@Aaronontheweb don't forget to delete the VersionSuffix line #7 in Directory.Build.props to remove the pre-release flag.


Edit: I see that the dev branch was tagged as 1.5.28 after this PR was merged. I think it makes a lot of sense to publish this build as 1.5.28 to NuGet.

@milang milang deleted the nunit4 branch September 9, 2024 18:04
@milang
Copy link
Contributor Author

milang commented Sep 16, 2024

@Aaronontheweb any chance you could publish Akka.TestKit.NUnit as version 1.5.28 (see my comments from last week) ?

Also, I noticed that Akka.TestKit.NUnit3 has not been published in a while, and is at version 1.3.8:
image

As per the changes in this PR, the latest build of Akka.TestKit.NUnit3 should be published as version 1.5.28. It can remain obsolete, but IMO the deprecation message should be updated from:

This version is now deprecated - please use Akka.TestKit.NUnit (which also supports NUnit 3)

to:

This version is now deprecated - please use Akka.TestKit.NUnit with NUnit 4.

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.

Update TestKit to use NUnit v4
5 participants