Thanks @manfred-brands I had been thinking a discussion would be good too. I have been intending to get back to these, including your other PR. It's late here now but hopefully tomorrow

@stevenaw My approach is as follows:

1. Leave existing EqualConstraint untouched. This is the fallback for everything not covered by the next steps.
2. Add new specific constraints for a subset of types.
   1.1. EqualStringConstraint for string. This then disallows non-valid modifiers like .Within
   1.1. EqualNumericConstraint for primitive types. This then disallows non-valid modifiers like .IgnoreCase and .Seconds. It also limits boxing. One left in for Arguments passed to base class and the EqualConstraintResult. Maybe I can remove those.
   1.1. DateTimeNumericConstraint for DateTime. This then allows WithIn(300).Milliseconds etc.

This means we reducing the amount of valid modifiers from the newer constraints.
It always bugged me that NUnit allows Assert.That(1, Is.EqualTo(2).IgnoreCase.Within(3).Hours)

Part of the problem is the disjoint between the Constraint and the Assert.
Especially the Apply<TActual> has no relation to the type of the Constraint.

I tried adding Assert.That<TActual>(TActual actual, EqualNumericConstraint<TActual> constraint) but the compiler complains about ambiguous methods when the two don't match like in: Assert.That(byteVariable, Is.EqualTo(1))

error CS0121: The call is ambiguous between the following methods or properties:
'Assert.That<TActual>(TActual, IResolveConstraint, NUnitString, string, string)' and 
'Assert.That<TActual>(TActual, EqualNumericConstraint<TActual>, NUnitString, string, string)'

Should TActual be byte or int?
Should the user be more clear and specify: Assert.That(byteVariable, Is.EqualTo((byte)1))

I like the idea of providing more strongly-typed constraints here with a more relevant subset of modifiers. I think we'd have to be conscious and mindful of breaking changes and that we may want to identify and decide on how to handle a few different scenarios up front. My thoughts from my own attempts:

To get the best experience with generic constraints, we'd want to extend Constraint itself (or potentially even a Constraint<T>) but I think this could cause subtle issues for existing consumers here as we'd be changing the return type of Is.EqualTo() from EqualsConstraint to something else which doesn't subclass. This however, would cause both binary and source breaking changes.

A few of the issues I could foresee which we'd want to navigate:

One break would be just usages of modifiers which "do nothing" and are currently ignored, like IgnoreCase on a numeric comparison. A test like this would currently pass but would fail to compile with a syntax error if we changed to a numeric-specific overload here which no longer inherited from EqualConstraint.

Assert.That(2, Is.EqualTo(2).IgnoreCase)

Another one could be that any change in return type which doesn't subclass the existing return type (ie: if we don't do NumericEqualsConstraint : EqualsConstraint) which be a binary breaking change for things like explicit target typing, conflicting return typing.

EqualConstraint x = Is.EqualTo(3);

This also gets tricky if we consider that a test or assertion framework may build on top of NUnit or individual consumers could have their own wrappers or aliases for things. Note that I haven't tested the below.

public class MyIs : Is {
  public static EqualConstraint EqualTo(int expected)
  {
      return Is.EqualTo(expected);
  }
}

global static Is = MyIs;

A third thing I just noticed now is internally we also have a few references to EqualConstraint which would need to be duplicated for any specialized equals constraint. This could, of course, be worked around how we may want to be mindful that specializing these constraints should, ideally, be easy on maintenance too:

@manfred-brands I feel like we've been approaching this differently and I find design discussions always harder in PRs. How would you feel if we moved the discussion to an issue. #4845 may be a natural fit unless you wanted a separate issue to track your ideas for these specializations

From my own brief foray into this approach:

Assert.That<TActual>(TActual actual, EqualNumericConstraint<TActual> constraint)

I believe we'd need to take these changes right down to the resolver + builder to properly wire this all together, but I'd be very happy to be proven wrong 🙂

@stevenaw My aim is not for binary backward compatibility.

One break would be just usages of modifiers which "do nothing" and are currently ignored, like IgnoreCase on a numeric comparison. A test like this would currently pass but would fail to compile with a syntax error if we changed to a numeric-specific overload here which no longer inherited from EqualConstraint: Assert.That(2, Is.EqualTo(2).IgnoreCase)

This is more an aim than an oversight. At the moment EqualConstraint has every modifiers possible to cater for every possible type. The fact that these modifiers are possible, and ignored, on non-appropriate types is a unwanted feature in my opinion.
Some combinations throw and we even have unit test validating that. But if those that don't make sense no longer compile seems to be more valuable.

My set of 3 PRs don't intend to replace EqualConstraint with a generic one as we both tried and copying those inappropriate modifiers doesn't make sense. But to have special versions for Strings, Numerics and Time with modifiers only suitable for those types. The latter two also reduce boxing. To complete that I would need to change the ConstraintResult hierarchy to have version that allows T for ActualValue and an Constraint that allows T for Arguments. I'm not sure if I can slot that in without breaking things.

A third thing I just noticed now is internally we also have a few references to EqualConstraint which would need to be duplicated for any specialized equals constraint. This could, of course, be worked around how we may want to be mindful that specializing these constraints should, ideally, be easy on maintenance too:

I did notice that, because some tests failed, and I did modify it however I rather remove the need for this. If a constraint needs different formatting it should be done in the constraint itself. The current situation is already an issue for someone wanting to make separate EqualXXXConstraint outside of the framework. In this case the equal to could be added to EqualConstraint.Description allowing the removal of the test in the pointed out code.

This looks good to me too, though one question I have is how we'd want code like this to behave:

[Test]
public void MyTest() => Assert.That(1, Is.EqualTo(1).IgnoreCase);

A test like that will currently pass and the IgnoreCase modifier ignored. Is it ok to introduce a compiler error for such a (rare?) case? Or, if the test assembly has a lower version of nunit and in turn references a third party dependency with a higher version of nunit, a MissingMethodException would be thrown.

I suspect these cases are rare, but likely exist in some consumers and would be worth at least discussing to confirm.

Imho, compiler error.

Such code is nonsense, and the right thing to do is to not accept it, thus compiler error.
How we get there, it all depends, and it might be NUnit5.

The latter two also reduce boxing. To complete that I would need to change the ConstraintResult hierarchy to have version that allows T for ActualValue and an Constraint that allows T for Arguments. I'm not sure if I can slot that in without breaking things.

I am fine with breaking things. WHEN we see it is breaking, we should just PR into a vNext branch, OR, even better, branch out now a 4.X branch and let main be the next major version.
We do have a v3 branch for that release version, so a v4 branch would be appropriate.

If we are fine with the change in return typing merging into v4 but not the compiler error being introduced then an option could be to add the irrelevant properties back onto these type-specific constraints and to immediately mark them [Obsolete] to warn people. We could then remove the properties in vNext.

@stevenaw I am starting to think that statements that are not correct in a C# context should be considered a bug, not a breaking change. When we say that Assert.That(42,Is.EqualTo(42).IgnoreCase then adding the IgnoreCase doesn't make sense. If that had been any normal C# piece of code, it would not compile.
So if we regard these as bugs (same with Within(1) when applied to strings, then this change is not a breaking change, but a bugfix.

I don't think our [documentation[(https://docs.nunit.org/articles/nunit/writing-tests/constraints/EqualConstraint.html?q=EQualTo) states anywhere that this is a feature :-) .

And our Analyzer give a NUnit2021 warning for this. In my view, that is ample warning.

What do you think ?

I agree it could be considered a bug as we should be guiding users away from API misuse. I would argue that a case like this where we know at compile time that a modifier isn't applicable to the arguments could meet that threshold.

I'm a little more hesitant to introduce a compile or runtime error as a "fix" outside of a semver-major bump, however I'd be ok with that if we think it would be rare for this to come up.

On the analyzers note, it's true that we have analyzers to help here, which likely makes it rarer as well, however they are separate packages and not everyone will have installed both. I suspect however, that those on Nunit 4 are also much more likely to have the analyzers installed given we advised their use during the upgrade path from NUnit 3. So perhaps it truly would be quite rare for modifiers like IgnoreCase to be used for numeric comparisons in an NUnit 4 codebase.

@manfred-brands @mikkelbu @OsirisTerje I'm not sure if I'm missing anything in this risk analysis, but it sounds like we are leaning towards this going into v4? Thoughts?

I searched github for language:csharp ").IgnoreCase and found 1.8K code results.

I then asked ChatGpt to search github with the same query for me and check the amount of numerical cases. The result I got was:

Upon reviewing the search results for language:csharp ").IgnoreCase" on GitHub, the vast majority of instances involve string operations, such as case-insensitive comparisons or regular expressions. However, there are a few cases where IgnoreCase is applied to non-string types, like numeric comparisons. These instances are rare and typically result from misunderstandings or errors in the code, as IgnoreCase is intended for string operations

For example, in some codebases, you might find assertions like:

Assert.That(123, Is.EqualTo(123).IgnoreCase);

This usage is semantically incorrect because numeric values don't have case distinctions. Such cases are uncommon and usually indicate a need for code review or refactoring to ensure appropriate use of IgnoreCase.

In summary, while IgnoreCase is predominantly used correctly with strings, occasional misapplications to non-string types do occur, though they are not widespread.

Our release notes should clearly specify that incorrect usage, which previously went unreported (excluded the analyzer which did report it) and compiled successfully, will no longer compile. We should also provide a detailed list of the specific cases affected.

We should also raise appropriate bug issues to point back to, which are then fixed by these PRs.

From a release notes perspective, a type-safe constraint system could also be considered a "feature" even if it is just for equality checks on primitives at the moment

@stevenaw Interesting thoughts! But if we create a new syntax, then we're in breaking water, right? So, can we do this with only fixing missing compiler errors ?

I noticed in earlier comments, either from you or @manfred-brands about the disconnect between the actual value and the constraint, as in Assert.That(actual,constraint), where the constraint doesn't really know about the type of the actual. Whereas if we change the syntax to Assert.That(actual).constraint they will be connected. And that would be breaking if we do that as a change, but if we do that as an addition to the existing, then we're in the clear.

Interesting thoughts! But if we create a new syntax, then we're in breaking water, right? So, can we do this with only fixing missing compiler errors ?

On the syntax front, I think it depends. If type-safe constraints required a new syntax then it could be non-breaking if it were possible to do in a backwards compatible way. The constraint syntax was, after all, introduced in a minor version bump for NUnit 2. Though I suspect that any future change like that may still be large enough and require enough discussions and consideration to wait for a vNext anyways.

@stevenaw

an option could be to add the irrelevant properties back onto these type-specific constraints and to immediately mark them [Obsolete] to warn people. We could then remove the properties in vNext.

No absolutely not.

if the test assembly has a lower version of nunit and in turn references a third party dependency with a higher version of nunit, a MissingMethodException would be thrown.

NuGet won't allow this. It will say that the 3rd party needs a higher version of nunit and you need to update.

If it is the other way around, like the 3rd party is compiled with an older version, it is not an issue.
The pre-existing code would have been compiled into the overarching EqualConstraint which I deliberately have not touched. If re-compiled it would become an EqualNumericConstraint and result correctly in a syntax error.

On the analyzers note, it's true that we have analyzers to help here, which likely makes it rarer as well, however they are separate packages and not everyone will have installed both

Others, xUnit and Tunit bundle them.
Could we create a new nuget package nunit.framework which contains what now is the nunit nuget package.
Then change the existing nunit package to be empty with dependencies on both nunit.framework and nunit.analyzers.

@OsirisTerje Regarding syntax.

Assert.That<TNumber>(TNumber, Constraint<TNUmber>) might work, but likely needs to go in a vNext branch.

Assert.That<TNumber>(TNUmber) would mean we will be copying TUnit syntax.
Not sure how that fits in with copyright (MIT) and if we want to have the same syntax would it be better to join forces than compete?

Assert.That<TNumber>(TNUmber) would mean we will be copying TUnit syntax.
Not sure how that fits in with copyright (MIT) and if we want to have the same syntax would it be better to join forces than compete?

This answer is rather long, but I thought it worthwile to elaborate on this in one go, so that we have it clearly stated.

TUnit is building their syntax as a variation of the NUnit syntax. It is very close to the NUnit syntax, more than XUnit. That applies to not only the assert and constraint syntax, but also to the attributes they have. They have done variations on the syntax, like the use of fluent notation for the Assert.That and constraint. TUnit are stating that they are inspired by NUnit and XUnit.

For me, I have no objections to others being inspired by our work, the MIT license we have allows that.

Fluent notation is well supported in FluentAssertions and Shouldly, which I assume are also inspirations.

NUnit has its own syntax for assertions, constraints, and attributes, giving us the freedom to make changes as we see fit. The changes we’re discussing are minimal, and the basic syntax will remain the same. We should only proceed if these changes add clarity or make our work easier. At the same time, we shouldn't feel restricted from modifying our syntax simply because others have created variations of it.

Also, XUnit grabbed our Assert.Multiple a year ago or so. That was just cool! We grabbed their idea of class instance by test (using the Lifecycle attribute).

I believe the TUnit team is well aware that they can contribute and 'join forces' with us; however, from what I gather, they prefer to start fresh. This approach has obvious benefits, but it also means they have a long journey ahead to make TUnit a complete test framework. I have no doubt that they are fully capable of achieving that.

To summarize

1. MIT License on Ideas and Syntax

The MIT license focuses on the code itself rather than ideas, syntax, or patterns. It allows others to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the code, , provided attribution is maintained according to the license.

Syntax and ideas, such as Assert.That or fluent notation, generally don’t fall under copyright restrictions. Copyright protects the exact implementation (code), not the abstract concepts or patterns.

Inspiration from a syntax or a similar structure is perfectly acceptable under MIT. Therefore, TUnit have not breached our MIT license. And likewise, if we adopt some of their's, or FluentAssertions, or Shouldly's ideas, we have not broken theirs.

2. Freedom to Modify Syntax

Since NUnit created and owns its syntax, it retains full control to change or expand it as desired. The MIT license does not limit NUnit’s ownership or rights over its own syntax.

This means we can freely modify or adapt NUnit's syntax, regardless of any variations introduced by others, such as TUnit. The license does not restrict us based on others' adaptations, and we do not lose ownership of our syntax or the flexibility to adjust it.

3. Open Source Practice of Idea Sharing

Open source projects regularly share ideas and even specific design patterns; adopting useful approaches from each other is common practice.

Examples like xUnit adopting Assert.Multiple and NUnit adopting class-per-test lifecycle patterns illustrate the spirit of collaboration in open source. As pointed out above, these adaptations are generally celebrated rather than contested.

4. Legal Standing and Ownership

NUnit remains on solid ground from a legal and ownership perspective. Using syntax inspired by NUnit, like TUnit has, is acceptable under the MIT license. NUnit retains its rights and can evolve its syntax independently.

Since no code has been copied directly (only the syntax and ideas), TUnit’s approach aligns with open-source norms. (They could, if they so prefer, also copy code if they like, but then have to include our copyright notice.) NUnit’s ownership of its syntax is unaffected, and any changes it makes won’t impact TUnit’s use of similar syntax or patterns.

The MIT license’s permissive nature ensures that the original authors (NUnit, in this case) retain full control over their code and its future development. So regardless of what TUnit does, NUnit still owns our code and can change as we want, even if there are similarities.

Could we create a new nuget package nunit.framework which contains what now is the nunit nuget package.
Then change the existing nunit package to be empty with dependencies on both nunit.framework and nunit.analyzers.

Good idea!
It may be breaking for those who use NUnit as a library, possibly - or possibly not, they would only get the analyzer in addition. Should we do this in V4 or V5 ?
We should ensure we're not touching anything else, and it is important we keep the statistics for NUnit package.
Doing this we should also look into how a new version of NUnitLite would fit into this, moving us in the direction of the new Test Platform.

PS: We're going a bit off topic on this PR, right? Should we move these things into a discussion issue ?

If it is the other way around, like the 3rd party is compiled with an older version, it is not an issue.
The pre-existing code would have been compiled into the overarching EqualConstraint which I deliberately have not touched. If re-compiled it would become an EqualNumericConstraint and result correctly in a syntax error.

Thanks @manfred-brands this was indeed the situation I had been thinking and just mistyped. However, I'm also glad you thought about the implications and mentioned them here! It's good to have these sort of design decisions documented.

Due to merge conflicts superseded by combined PR #4882