Breaking change in version 4.4.0-beta.1

[OsirisTerje]

@manfred-brands @stevenaw

I ran the beta version on some production code, which use a test library that depends on NUnit.4.3.2, and got two tests failing due to MissingMethod exceptions.

This seems to come from changes in the EqualConstraint, and might be from PR #4925 .

I ran it through ChatGPT (o4-mini-high), and asked it to figure out what changes may have caused it, and got some results (which may or may not be the correct one though). The ChatGPT response is at the bottom here.

The failing tests came out with these messages and stacktraces:

Message: 
System.MissingMethodException : Method not found: 'NUnit.Framework.Constraints.EqualConstraint NUnit.Framework.Constraints.ConstraintExpression.EqualTo(!!0)'.

  Stack Trace: 
ClientCredentialKeyPairsConfigConsistencyTests.ThatClientIdForProductionIsDifferentThanTest()
RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
1)    at Fhi.ClientCredentials.TestSupport.ClientCredentialKeyPairsConfigConsistencyTests.ThatClientIdForProductionIsDifferentThanTest()
RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

The Assert line that fails is

Assert.That(clientCredentialsConfigurationForProduction!.ClientId, Is.Not.EqualTo(clientIds.First()), "ClientId for production is equal to clientId used for tests");

This is a breaking change, there might also be others, since we have touched more than the EqualConstraint, e.g. SomeItemsConstraint has also been touched.
However, for those just updating and do not have mixed packages, the update is non-breaking (I believe).

We have to decide how to resolve it:

1. Just mark the PR as breaking, and list it in the release notes as a breaking change, but keep it as version 4.4.0.

PRO: Easy fix, can document exactly HOW it breaks, and what to be aware of

CON: Breaks SemVer, and might confuse users who expect a minor version to not introduce breaking changes.

2. Update to version 5.0.0

PRO: Marks it as a breaking change release, follows SemVer

CON: These are kind of inadvertently breaking changes, and very few - might be only one.

3. Introduce a shim method to cover it

PRO: Will make the release non-breasking

CON: Might not be easy, seems it is the return value that is the issue.

Vote ?

---

Chat GPT response:

[stevenaw]

I tend to lean towards favouring compatibility so I tend to lean towards seeing what we can do to make this non-breaking.

I say "leaning towards" instead of "vote" as I am on vacation and without a laptop to poke at this code to understand the issue better.

I'm also generally hesitant to jump to 5.0.0 over this if we can help it as there's other planned breaking changes we also want to do in the future and it might be smoother overall if breaking changes could be grouped together into planned major version bumps. I feel like we did a great job identifying and communicating breaking changes during the 4.0.0 release in large part because we could plan for them and identify migration paths.

[manfred-brands]

Can we run a github action against every PR to check if it breaks binary compatibility?
Or maybe we should have a binary dll compiled against the previous release and run the tests in there.

I thought I had this backward compatible as EqualConstraint<T> derives from EqualConstraint.
So anybody expecting the latter would work.

Let me try if I can make it "binary compatible".

[OsirisTerje]

@manfred-brands @stevenaw After struggling to create a good repro for this error, I realized the code actually had a bug, so the test compares a string with a class instance - which obviously will never be equal.

The repro is not cleaned up, I first added tests for all constraints (using CoPilot), but they all worked.

I then added some more specific tests which also all worked.

Then when adding the package that actually have the asserts, I had the error and noticed the bug there.

I could then add a specific repro for that, which also show the error.

The repro code that fails is here: https://github.com/nunit/nunit.issues/blob/18d9cdcce0f789d92466ce676e4a6fd437db6da8/Issue5011/currentversion/ClientCredentialsTests.cs#L16 using NUnit 4.4.0-beta.1 and refers to the lastversion project as a assembly reference.

The project lastversion refers to NUnit 4.3.2, the code there works: https://github.com/nunit/nunit.issues/blob/18d9cdcce0f789d92466ce676e4a6fd437db6da8/Issue5011/lastversion/ReproClientTest.cs#L12

❓ So, I am not sure if the @manfred-brands PR should be merged, as it also introduced the EqualToGeneric. Or are there other aspects to this?

In the library with the bug (https://github.com/FHIDev/Fhi.ClientCredentials/blob/e42246d29ec9e7c2a668e9b0b6268bea8be3dc63/Fhi.ClientCredentials.TestSupport/ClientCredentialKeyPairsConfigConsistencyTests.cs#L78) , the NUnit.Analyzers package was not included in the project .

If the analyzer are included, then you will get NUnit2021, which is an Error.

❓ It seems to me that the analyzer is becoming more and more important, so perhaps it should be included as a dependence in the NUnit.Framework package ?

[OsirisTerje]

@manfred-brands

Can we run a github action against every PR to check if it breaks binary compatibility?
Or maybe we should have a binary dll compiled against the previous release and run the tests in there.

The repro has a bunch of tests that does this. We can separate these out and run them in an action, as you suggest

[stevenaw]

Thanks for providing the repro @OsirisTerje

❓ So, I am not sure if the @manfred-brands PR should be merged, as it also introduced the EqualToGeneric. Or are there other aspects to this?

I agree with talking through the tradeoffs here as I am also inclined to avoid introducing EqualToGeneric here. It sounds like this case for the MissingMethodException is likely to only happen with non-typesafe comparisons. Some of these may be comparisons which always fail, such as comparing a class and a string, in which case their presence in test suites is likely to only happen if combined with the Not constraint (effectively turning an always failing test into always passing) and also in projects which don't reference the Analyzers. Alerting about such cases is arguably a good thing.

One other area for consideration here is with comparisons expected to pass when comparing two different types. NUnit will perform convenience conversions for these cases within the constraints. I haven't testing it but could these also experience a MissingMethodException?

It seems to me that the analyzer is becoming more and more important, so perhaps it should be included as a dependence in the NUnit.Framework package ?

I agree with this. There have been a few past discussions about this too. I believe one main outstanding consideration was if some analyzer rules were more opinionated or special-cased than others. A follow-up to that was then about when such a change should be introduced and if it should warrant a semver-major bump such as NUnit 5.

I can't find the discussion at the moment, however. Perhaps @manfred-brands or @mikkelbu may recall

[mikkelbu]

I can't find the discussion at the moment, however. Perhaps @manfred-brands or @mikkelbu may recall

Not at the moment, but I also have very limited time for open source currently :)

There were some "bundling" discussions in the original Analyzer thread - #629 (e.g. #629 (comment) and following comments), but I think this is before my time in NUnit (or close to the beginning), and this is a long time ago, so both NUnit and the Analyzers have changed a lot since then.

I think that there have been some people that didn't like the Analyzers:

• e.g. the suggestions to use the constraint model instead of the classic/legacy model - see e.g. Clarifying the status of the classic model assertions #3688 (comment)
• or Analyzer message NUnit2005 is annoying #4401

Personally, I don't mind adding the analyzers as a dependency of the framework, but I think it should be a semver-major bump

[manfred-brands]

@stevenaw I have no problem dropping the EqualToGeneric, it was a forced name to allow generics.

It sounds like this case for the MissingMethodException is likely to only happen with non-typesafe comparisons.

This is NOT the case, it will happen for all comparisons where the type has not its own EqualTo overload.

Anything that triggers the generic EqualTo<T> overload:

Such as for Guid or any record type or any user's class implementing IEquatable<T>

    [Test]
    public void TestGuid()
    {
        Guid g = Guid.NewGuid();
        Guid gg = g;
        Assert.That(gg, Is.EqualTo(g));
    }

In addition the MissingMethod will trigger with any collection checks:

    [Test]
    public void TestList()
    {
        List<int> l = [1, 2, 3];
        IEnumerable<int> e = l;
        Assert.That(l, Is.EqualTo(e));
    }

The PR restores the original declared return type for EqualTo<T> back to EqualConstraint to match the 4.3.2 release.
Ensuring that the above still run when using a 4.4.x binary.

To allow the typed UsingPropertiesComparer the PR adds a generic method to the non-generic EqualConstraint.
Moving the generic parameter from the class to a new method which require specification of the type, even though we knew it but lost it.

Note that the above example tests don't trigger any NUnit.Analyzer warnings as it compares variables with matching types.

• Compile the below project: dotnet build
• Test it: dotnet test --no-build
• Copy nunit 4.4's src/NUnitFramework/framework/bin/Debug/net8.0/nunit.framework.dll to the bin/Debug/net8.0 folder.
• Test it against the updated dll: dotnet test --no-build

That will fail with:

TestGuid (7ms): Error Message: System.MissingMethodException : Method not found: 'NUnit.Framework.Constraints.EqualConstraint NUnit.Framework.Is.EqualTo(!!0)'.
TestList (< 1ms): Error Message: System.MissingMethodException : Method not found: 'NUnit.Framework.Constraints.EqualConstraint NUnit.Framework.Is.EqualTo(System.Collections.Generic.IEnumerable`1<!!0>)'.

• Compile my PR branch and copy the nunit.framework.dll binary again to the test project.
• Test it against the updated dll: dotnet test --no-build

It will now pass again.

Compatibility.zip

[OsirisTerje]

This is NOT the case, it will happen for all comparisons where the type has not its own EqualTo overload.

Thanks @manfred-brands ! I was wondering if there was something more lurking around here :-)

But that means we still need the EqualToGeneric method, right ?
And we could hit more of these for other constraints too ? E.g. EquivalentTo, SubsetOf, SupersetOf ?

[manfred-brands]

But that means we still need the EqualToGeneric method, right ?

It is either:
Is.EqualToGeneric(instance).UsingPropertiesComparer(c => c or
Is.EqualTo(instance).UsingPropertiesComparer<ClassWithSomeToleranceAwareMembers>(c => c

It is strictly speaking not needed, but prevents the need for the user to specify the type in the UsingPropertiesComparer overload.
We can drop it and for V5.0 change the return type back to the generic EqualConstraint<T>.

And we could hit more of these for other constraints too ? E.g. EquivalentTo, SubsetOf, SupersetOf ?

No. All these didn't have a generic overload, so it is a new function.
Code compiled with 4.3.2 will call the existing object overload.
New code will call the generic overload.

[OsirisTerje]

Got it.

It is either:
Is.EqualToGeneric(instance).UsingPropertiesComparer(c => c or
Is.EqualTo(instance).UsingPropertiesComparer(c => c

I then think I would prefer the latter.

UPDATE: I am not sure................which one.......................have the least impact............

[manfred-brands]

Ok, I have removed the EqualToGeneric methods.

. UPDATE: I am not sure................which one.......................have the least impact............

It is users that want to use the new PropertiesComparerConfiguration to exclude properties have to specify the type.
The downside is that there is no compile time check that the properties listed actually match the instance being tested.

We can change the return type back to the generic one for the next breaking release.