Upgrade NUnit to v4; misc test improvements, #1001#1271
Draft
paulirwin wants to merge 8 commits intoapache:masterfrom
Draft
Upgrade NUnit to v4; misc test improvements, #1001#1271paulirwin wants to merge 8 commits intoapache:masterfrom
paulirwin wants to merge 8 commits intoapache:masterfrom
Conversation
Contributor
Author
|
Marking as draft until NUnit 4.6 is released with my merged PR (nunit/nunit#5236) included, to remove the reflection for ArgDisplayNames. However, the rest is still able to be reviewed. cc @NightOwl888 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Upgrade NUnit to v4; misc test improvements.
Fixes #1001
Description
This PR upgrades NUnit to v4. For more details on some of the migration, see the docs here: https://docs.nunit.org/articles/nunit/release-notes/Nunit4.0-MigrationGuide.html
NUnit 4 dropped support for netstandard2.0; thus, this PR drops support for it from the TestFramework project. This only really would affect users who need to create an intermediate unit testing library (note: not a unit test project) that targets netstandard2.0 and references TestFramework. We expect this to not have a wide impact given current relatively low usage of TestFramework on NuGet (~100 downloads/mo). But if this is you, there is a simple workaround: either choose a single supported target (i.e. net8.0 or net462), or multi-target (i.e.
<TargetFrameworks>net462;net8.0</TargetFrameworks>in your project). Getting Lucene.NET on NUnit 4 will be more impactful in the long run and less restrictive to users than having to support a limited set of netstandard2.0 runtimes that are still in use (i.e. maybe Mono and some versions of Unity), and again this is just for unit tests.My first pass at this added a global using alias for Assert to point it to our Lucene.Net.TestFramework.Assert class, but that created way too many files changed in this PR, and it can be added later. So while there are some namespace cleanups here, I tried to minimize the amount of files changed since the other stuff is more important. Also, this upgrade exposed several places where we were not using our Assert class, so that has been fixed in several places.
One notable breaking change in NUnit 4.5 and later is that it now throws at runtime if you try to use the TimeoutAttribute on modern .NET. This was previously documented as not working on .NET 5 and later, but now it is an exception. To solve this at the class/test level, it was replaced with CancelAfterAttribute, which will trigger a CancellationToken parameter if the test runs longer than the configured value. So CancellationToken support was added to most of these tests and threaded through to calls where it made sense to do so. Only one test did not have an obvious seam since ForceMerge is not currently cancelable.
For TimeoutAttribute at the assembly level, there is not a good replacement. So I've removed it, since the existing
--blame-hang-timeoutlikely already covers this for both .NET Framework and modern .NET.Assert saw many documentation and correctness improvements in this PR; too many to mention. Notably, it switched to XML doc comments. Additionally, many unit tests were added for the NUnit-derived attributes.
The private/internal ArgDisplayNames property in NUnit changed to be an auto-prop from a declared field, so the reflection code that pokes into that has been updated to get the internal property instead of the private field. Unit tests were also added for this functionality to help detect and prevent regressions. Although if this regresses with a newer version of NUnit, the code should safely fall back to just using the ToString representation of the class fixture arguments, if anyone is using that (note that we are not using this functionality in our code).
Finally, a concurrency bug in TestRAMDirectory was fixed (which might have been surfaced by the NUnit 4 upgrade), which has the benefit of better matching the upstream Java code anyways. The root case was two different runs of the test could be writing to the folder at the same time.
AI: This was largely done by hand, but some repetitive parts (like removing usings) and some unit tests were done with the help of Claude Code.