aherbert commented on code in PR #227:
URL: https://github.com/apache/commons-geometry/pull/227#discussion_r1499188481
##########
commons-geometry-euclidean/src/main/java/org/apache/commons/geometry/euclidean/threed/Vector3D.java:
##########
@@ -339,6 +339,7 @@ public Vector3D reject(final Vector3D base) {
* frame with one of the axes in a predefined direction. The
* following example shows how to build a frame having the k axis
* aligned with the known vector u :
+ *
Review Comment:
Undo this formatting.
##########
commons-geometry-euclidean/src/test/java/org/apache/commons/geometry/euclidean/threed/Vector3DTest.java:
##########
@@ -1146,6 +1145,65 @@ void testEqualsAndHashCode_signedZeroConsistency() {
Assertions.assertEquals(b.hashCode(), d.hashCode());
}
+ @Test
+ void testHashCodeCollisions_symmetricAboutZero() {
+ final double any = Math.random();
Review Comment:
Note that this could create sporadic failures. We can formalise this using
some random numbers:
```java
@ParameterizedTest
@ValueSource(doubles = {1.23, 4.56, 42, Math.PI})
void testHashCodeCollisions_symmetricAboutZero(double any) {
```
##########
commons-geometry-spherical/src/test/java/org/apache/commons/geometry/spherical/twod/GreatCircleTest.java:
##########
@@ -664,7 +664,7 @@ void testHashCode() {
Assertions.assertEquals(hash, a.hashCode());
Assertions.assertNotEquals(hash, b.hashCode());
- Assertions.assertNotEquals(hash, c.hashCode());
+ Assertions.assertEquals(hash, c.hashCode());
Review Comment:
If this is testing the hash codes are equal (because the objects are equal)
then we should test the objects are equal:
```java
// Equal objects have equal hash codes
Assertions.assertEquals(a, c);
Assertions.assertEquals(hash, c.hashCode());
```
##########
commons-geometry-euclidean/src/test/java/org/apache/commons/geometry/euclidean/threed/Vector3DTest.java:
##########
@@ -23,7 +23,6 @@
import java.util.Comparator;
import java.util.List;
import java.util.regex.Pattern;
-
Review Comment:
Undo this formatting.
##########
commons-geometry-euclidean/src/test/java/org/apache/commons/geometry/euclidean/threed/Vector3DTest.java:
##########
@@ -1146,6 +1145,65 @@ void testEqualsAndHashCode_signedZeroConsistency() {
Assertions.assertEquals(b.hashCode(), d.hashCode());
}
+ @Test
+ void testHashCodeCollisions_symmetricAboutZero() {
+ final double any = Math.random();
+ final double neg = -any;
+ final double pos = +any;
+
+ final Vector3D negX = Vector3D.of(neg, 0.0, 0.0);
+ final Vector3D posX = Vector3D.of(pos, 0.0, 0.0);
+ final Vector3D negY = Vector3D.of(0.0, neg, 0.0);
+ final Vector3D posY = Vector3D.of(0.0, pos, 0.0);
+ final Vector3D negZ = Vector3D.of(0.0, 0.0, neg);
+ final Vector3D posZ = Vector3D.of(0.0, 0.0, pos);
+
+ int xNegHash = negX.hashCode();
+ int xPosHash = posX.hashCode();
+ int yNegHash = negY.hashCode();
+ int yPosHash = posY.hashCode();
+ int zNegHash = negZ.hashCode();
+ int zPosHash = posZ.hashCode();
+
+ String xMessage = String.format("negXHash: %s, posXHash: %s (expected
to be non-equal)\n", xNegHash, xPosHash);
+ String yMessage = String.format("negYHash: %s, posYHash: %s (expected
to be non-equal)\n", yNegHash, yPosHash);
+ String zMessage = String.format("negZHash: %s, posZHash: %s (expected
to be non-equal)\n", zNegHash, zPosHash);
+
+ Assertions.assertNotEquals(zNegHash, zPosHash, zMessage);
+ Assertions.assertNotEquals(yNegHash, yPosHash, yMessage);
+ Assertions.assertNotEquals(xNegHash, xPosHash, xMessage);
+ }
+
+ @Test
+ void testHashCodeCollisions_symmetricAboutArbitraryValue() {
Review Comment:
Same as testHashCodeCollisions_symmetricAboutZero. Use a ParameterizedTest
and simplify the assertion message.
```java
@ParameterizedTest
@CsvSource(
"1.23, 72.68",
// etc
)
void testHashCodeCollisions_symmetricAboutArbitraryValue(double any, double
arb) {
```
##########
commons-geometry-euclidean/src/test/java/org/apache/commons/geometry/euclidean/threed/Vector3DTest.java:
##########
@@ -1146,6 +1145,65 @@ void testEqualsAndHashCode_signedZeroConsistency() {
Assertions.assertEquals(b.hashCode(), d.hashCode());
}
+ @Test
+ void testHashCodeCollisions_symmetricAboutZero() {
+ final double any = Math.random();
+ final double neg = -any;
+ final double pos = +any;
+
+ final Vector3D negX = Vector3D.of(neg, 0.0, 0.0);
+ final Vector3D posX = Vector3D.of(pos, 0.0, 0.0);
+ final Vector3D negY = Vector3D.of(0.0, neg, 0.0);
+ final Vector3D posY = Vector3D.of(0.0, pos, 0.0);
+ final Vector3D negZ = Vector3D.of(0.0, 0.0, neg);
+ final Vector3D posZ = Vector3D.of(0.0, 0.0, pos);
+
+ int xNegHash = negX.hashCode();
+ int xPosHash = posX.hashCode();
+ int yNegHash = negY.hashCode();
+ int yPosHash = posY.hashCode();
+ int zNegHash = negZ.hashCode();
+ int zPosHash = posZ.hashCode();
+
+ String xMessage = String.format("negXHash: %s, posXHash: %s (expected
to be non-equal)\n", xNegHash, xPosHash);
+ String yMessage = String.format("negYHash: %s, posYHash: %s (expected
to be non-equal)\n", yNegHash, yPosHash);
+ String zMessage = String.format("negZHash: %s, posZHash: %s (expected
to be non-equal)\n", zNegHash, zPosHash);
+
+ Assertions.assertNotEquals(zNegHash, zPosHash, zMessage);
Review Comment:
Why are these in z, y, x order? Makes more sense to use x, y, z, order.
##########
commons-geometry-euclidean/src/test/java/org/apache/commons/geometry/euclidean/twod/Vector2DTest.java:
##########
@@ -912,6 +911,53 @@ void testHashCode() {
Assertions.assertEquals(Vector2D.of(0, Double.NaN).hashCode(),
Vector2D.of(Double.NaN, 0).hashCode());
}
+ @Test
+ void testHashCodeCollisions_symmetricAboutZero() {
Review Comment:
Same as previous. Use a ParameterizedTest and simplify the assertion message.
##########
commons-geometry-euclidean/src/test/java/org/apache/commons/geometry/euclidean/threed/Vector3DTest.java:
##########
@@ -1146,6 +1145,65 @@ void testEqualsAndHashCode_signedZeroConsistency() {
Assertions.assertEquals(b.hashCode(), d.hashCode());
}
+ @Test
+ void testHashCodeCollisions_symmetricAboutZero() {
+ final double any = Math.random();
+ final double neg = -any;
+ final double pos = +any;
+
+ final Vector3D negX = Vector3D.of(neg, 0.0, 0.0);
+ final Vector3D posX = Vector3D.of(pos, 0.0, 0.0);
+ final Vector3D negY = Vector3D.of(0.0, neg, 0.0);
+ final Vector3D posY = Vector3D.of(0.0, pos, 0.0);
+ final Vector3D negZ = Vector3D.of(0.0, 0.0, neg);
+ final Vector3D posZ = Vector3D.of(0.0, 0.0, pos);
+
+ int xNegHash = negX.hashCode();
+ int xPosHash = posX.hashCode();
+ int yNegHash = negY.hashCode();
+ int yPosHash = posY.hashCode();
+ int zNegHash = negZ.hashCode();
+ int zPosHash = posZ.hashCode();
+
+ String xMessage = String.format("negXHash: %s, posXHash: %s (expected
to be non-equal)\n", xNegHash, xPosHash);
Review Comment:
These should be passed a suppliers to Assertions. However the Assertions
will already have a statement containing the two values and that they were
expected to be not equal. So we can just use:
```java
Assertions.assertNotEquals(zNegHash, zPosHash, "+/- z hash");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
