Implement CollectionHelper.GetHashCode that accepts IEqualityComparer<T> #1694
Conversation
- Also fix CollectionHelper.GetHashCode to account for the length of the collections
| @@ -205,18 +205,5 @@ public void NativeSQLQuerySpecificationInequalityOnSpaceLengthes() | |||
| Assert.IsFalse(spec1.Equals(spec2)); | |||
| Assert.IsFalse(spec2.Equals(spec1)); | |||
| } | |||
|
|
|||
| [Test] | |||
| public void NativeSQLQuerySpecificationInequalityOnSpaceOrderings() | |||
There was a problem hiding this comment.
This test is removed because.... the hash set does not guarantee the ordering.
There was a problem hiding this comment.
The new overload lacks tests. Tests similar to these tests should be added.
To be squashed.
|
|
| Is.EqualTo(CollectionHelper.GetHashCode(d2, KvpStringComparer.Instance))); | ||
| } | ||
|
|
||
| // Failure of following tests is not an error from GetHashCode semantic viewpoint, but it causes it |
There was a problem hiding this comment.
Better to replace asserts with warnings then
There was a problem hiding this comment.
The trouble with such warnings is that their failures will likely be unnoticed. A test that may just generate warnings without having any assert does no good in my opinion.
Even if the GetHashCode semantics allows collision, having collisions on simple and recurring cases is a bad thing and is worth a failing test case in my opinion.
There was a problem hiding this comment.
NUnit’s warnings mark tests as inconclusive.
There was a problem hiding this comment.
Yes but who checks inconclusive tests? If the build does not fail, a PR causing a regression on this point is very likely to not be rejected due to causing that regression, because it will likely goes unnoticed.
This is an alternative implementation to #1613