darkma773r commented on a change in pull request #112:
URL: https://github.com/apache/commons-geometry/pull/112#discussion_r539795480
##########
File path:
commons-geometry-euclidean/src/test/java/org/apache/commons/geometry/euclidean/threed/Bounds3DTest.java
##########
@@ -125,29 +125,12 @@ public void testFrom_invalidBounds() {
final Vector3D negInf = Vector3D.of(1, 1, Double.NEGATIVE_INFINITY);
// act/assert
- GeometryTestUtils.assertThrows(() -> {
- Bounds3D.from(Vector3D.NaN);
- }, IllegalStateException.class, INVALID_BOUNDS_PATTERN);
-
- GeometryTestUtils.assertThrows(() -> {
- Bounds3D.from(Vector3D.POSITIVE_INFINITY);
- }, IllegalStateException.class, INVALID_BOUNDS_PATTERN);
-
- GeometryTestUtils.assertThrows(() -> {
- Bounds3D.from(Vector3D.NEGATIVE_INFINITY);
- }, IllegalStateException.class, INVALID_BOUNDS_PATTERN);
-
- GeometryTestUtils.assertThrows(() -> {
- Bounds3D.from(good, nan);
- }, IllegalStateException.class, INVALID_BOUNDS_PATTERN);
-
- GeometryTestUtils.assertThrows(() -> {
- Bounds3D.from(posInf, good);
- }, IllegalStateException.class, INVALID_BOUNDS_PATTERN);
-
- GeometryTestUtils.assertThrows(() -> {
- Bounds3D.from(good, negInf, good);
- }, IllegalStateException.class, INVALID_BOUNDS_PATTERN);
+ assertThrows(IllegalStateException.class, () ->
Bounds3D.from(Vector3D.NaN));
Review comment:
> I still think that messages don't give us much.
The assertions on the expected exception messages are important because they
verify not only what was thrown but why and ensure that the exceptions produced
by the library are accurate and useful. To remove these assertions after they
are already in place would be taking a step backward.
@singhbaljit, is correct in that I was confused on the 3rd parameter to the
JUnit 5 `assertThrows` method: I thought it contained the expected message of
the thrown exception and not simply a string to display when the assertion
failed. As such, `Assertions.assertThrows` is not the complete drop-in
replacement for `GeometryTestUtils.assertThrows` that I thought it was.
Moving forward, the cleanest approach I see here is the following:
1. Remove the two argument `GeometryTestUtils.assertThrows(Runnable,
Throwable)` method. All current invocations of this method can be directly
replaced with the standard JUnit method.
2. Rename `GeometryTestUtils.assertThrows(Runnable, Throwable, String)` and
`GeometryTestUtils.assertThrows(Runnable, Throwable, Pattern)` to
`assertThrowsWithMessage` for clarify and implement internally with JUnit
assertions, as described by @singhbaljit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
