Using fluent syntax unintentionally removed in 3.8

[ChrisMaddock]

From @BlythMeister on #2410
with regard to the example code, we had
Assert.That(myCollection, Contains.Item(expectedItem).Using(new MyCustomComparer()));
The .Using fluent method existed in 3.7, but was removed in 3.8.
I was unable to find an equivalent, so changed to
Assert.That(myCollection.Contains(expectedItem, new MyCustomComparer()));
Which was a simple enough change as a 1 off, but we had around 100 usages of that old syntax with different collection names, expected names as comparers (across different projects)

This change was unintentional - the removal of CollectionContainsConstraint wasn't intended to cause any breaking changes.

@mikkelbu did the below analysis of what happened:

@BlythMeister This is very unfortunate. Actually, @ChrisMaddock spotted the missing Using in aa19bd1 (see #2239 (comment)), so we added Using to the EqualConstraint in bd2091b. However, the entire PR did contain an error, so Contains.Item was again changed in 15c27e5 to return a ContainsConstraint instead of a EqualConstraint. And currently ContainsConstraint does not have a Using method.

@rprouse - will leave you to decide if this should be critical or not, possibly along with #2411 - I haven't looked into that one.

[mikkelbu]

Before implementing a solution to this I think we should have tests that cover all the functionality of CollectionContainsConstraint - as it were in version 3.7 - and then use these tests to ensure that we have kept all the behaviour. I'll try to look into writing these tests tonight (if I have time). The tests should also cover #2411.

[rprouse]

I updated the title of this issue to better reflect what it is. Feel free to reword if needed.

I think this could warrant another hotfix since I don't think there is a workaround. There is a hotfix for #2411, but we could include it if this one is going out.

As for unit tests, we have tons of syntax tests and constraints tests, but we lack many tests that purposefully chain constraints together. I was thinking we should add a series of fluent syntax tests that do just that. They could be simplistic tests that just set up what we know is passing data and exercise various combinations of constraints.

[CharliePoole]

@rprouse I think we once had a test that covered almost all the syntax, including chaining things together. We may have eliminated it at some point in favor of more targeted tests. If so, it seems like that wasn't such a good idea. 😞 I'll take a look.

[mikkelbu]

I've tried to go through all the changes done in #2239 and write tests for each change. All the tests pass in version 3.7.1, but in 3.8.1:

• 5 does not compile (due to 'SomeItemsConstraint' does not contain a definition for 'Using'), and
• 3 fails with System.ArgumentException : The actual value must be an IEnumerable

https://gist.github.com/mikkelbu/370fe6a735c41906dca1fad28282f19e

[ChrisMaddock]

That test file looks great Mikkel. 👍 I'm surprised there was so much missing from our test suite. 😞

[mikkelbu]

@nunit/framework-and-engine-team I've assigned this issue and #2411 to myself, and I have a technical question (or two).

• If these issues should be part of a hotfix should I then base my topicbranch on the release branch?
• Is it okay if I solve both issues using one topicbranch or should I rather use two separate topic branches?
• Should I also update CHANGES.md and increment the version number, or is this a job for the person responsible for creating the hotfix?

[rprouse]

@mikkelbu you can base your changes on the master branch. I can cherry-pick the changes for the release. It would be easier to fix both issues in one PR since they are related. It will also make cherry picking the changes easier. Don't worry about updating CHANGES.md or the version number, I do that as part of the hotfix.

[mikkelbu]

@rprouse I've added this issue to the 3.8.2 milestone.