Within usage analyzer + tests + codefix

[Antash]

Closes #344

[manfred-brands]

I added a PullRequest on top of your branch which you can merge into your branch. It fixes the CodeFix for your single use case, so you can continue with more nunit tests and add the documentation. Note that if you run all nunit tets you get a skeleton of the new documentation, rename the file and add the missing bits.

[Antash]

@manfred-brands many thanks! Will finish the work soon

[Antash]

@manfred-brands I think I'm ready with the changes, could you please take a look?

[Antash]

@manfred-brands thanks for the instant review! I've addressed points you raised.

[Antash]

@manfred-brands Thank you! Didn't know how to rebase without the conflict so 2 commits finally.

[mikkelbu]

@manfred-brands Thank you! Didn't know how to rebase without the conflict so 2 commits finally.

No problem. We can also do a "squash and merge" when merging this in, so we only get one commit.

[Antash]

@mikkelbu thank you for your comments, all addressed!

[mikkelbu]

Hi @Antash

I did some manual testing and found some issues (I should have thought of these when I did the review and I'm sorry for the very long comment - note that I've written an update below in bold).

We have some special comparers in NUnit - see https://github.com/nunit/nunit/tree/master/src/NUnitFramework/framework/Constraints/Comparers - that override the normal equality for certain types (mostly collections, tuples, dictionaries etc.). Several of these make use of the Tolerance parameter (and we have already handled Tuples, DateTimeOffsets, numeric in this PR). As far as I can tell we need to handle:

• Arrays (or more generally IEnumerables) where we use the tolerance in the comparison of the individual items
• IDictionary (note we only use the tolerance in the comparison of the values)
• DictionaryEntry (note we only use the tolerance in the comparison of the values)
• KeyValuePair (note we only use the tolerance in the comparison of the values)
• IStructuralEquatable (e.g. ImmutableArray as far as I can remember)

So for instance the following Within is currently wrongly marked by the analyzer

        [Test]
        public void TestArray()
        {
            var value = 5.5;
            var value2 = 6;
            Assert.That(new[] { value }, Is.EqualTo(new[] { value2 }).Within(2));
        }

but the test pass, as the tolerance is used in the comparison.

Similarly, the following test pass (so Within is wronly marked), as we use the tolerance on the values in dictionaries, dictionaryentries, and keyvalue paris)

        [Test]
        public void DictionaryEntryValue()
        {
            var a = new DictionaryEntry("key", 1);
            var b = new DictionaryEntry("key", 2);
            Assert.That(a, Is.EqualTo(b).Within(2));
        }

The mark of Within in the following is correct, as we don't use tolerance on strings

        [Test]
        public void DictionaryEntryKey()
        {
            var a = new DictionaryEntry(1, "key");
            var b = new DictionaryEntry(2, "key");
            Assert.That(a, Is.EqualTo(b).Within(2));
        }

Also note that we pass down the tolerance recursively, so the following is valid (although a bit contrived example)

        [Test]
        public void NestedCase()
        {
            var a = new KeyValuePair<string, KeyValuePair<string, KeyValuePair<string, int>>>(
                "key1", new KeyValuePair<string, KeyValuePair<string, int>>("Key2", new KeyValuePair<string, int>("Key3", 3)));
            var b = new KeyValuePair<string, KeyValuePair<string, KeyValuePair<string, int>>>(
                "key1", new KeyValuePair<string, KeyValuePair<string, int>>("Key2", new KeyValuePair<string, int>("Key3", 4)));
            Assert.That(a, Is.EqualTo(b).Within(2));
        }

So I think we need to examine the types recursively to figure out if the Within make sense (similarly to how the comparers work), so if we compare collections, we need to check if the collection elements are of a type that use tolerance (and probably handle object as a type that allows tolerance as well); if we check DictionaryEntries, we need to check if the value is of a type that use tolerance (and ignore the key) ...

EDIT: Or we could start with a simpler solution and just consider all the types mentioned above (IEnumerable, Arrays, IDictionary, KeyValuePair, DictionaryEntry, and IStructuralEquatable) as allowing Within and wait with the recursively check to later.

False positive for char

Note that we have a special comparison for Chars (and this does not make use of the tolerance). But currently the analyzer does not mark Within in the following test

        [Test]
        public void Char()
        {
            var a = 'a';
            var b = 'b';
            Assert.That(a, Is.EqualTo(b).Within(2));
        }

but the test fails with

      Expected: 'b' +/- 2
      But was:  'a'
      Off by:   1.0d

So I think we should also mark Within on chars.

[manfred-brands]

Replaced with #537