You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2020/12/16 15:31:05 UTC
[GitHub] [commons-geometry] arturobernalg opened a new pull request #121: [GEOMETRY-105] - Replace the try-catch pattern with assertThrows()
arturobernalg opened a new pull request #121:
URL: https://github.com/apache/commons-geometry/pull/121
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-geometry] darkma773r commented on a change in pull request #121: [GEOMETRY-105] - Replace the try-catch pattern with assertThrows()
Posted by GitBox <gi...@apache.org>.
darkma773r commented on a change in pull request #121:
URL: https://github.com/apache/commons-geometry/pull/121#discussion_r545543026
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/precision/EpsilonDoublePrecisionContextTest.java
##########
@@ -20,6 +20,7 @@
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
+
Review comment:
Nit: many files have this spurious new line added before the class definition. (IDE formatter?) Obviously, this does not affect the code but let's remove them to avoid whitespace-only changes.
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -50,61 +51,28 @@ public static void assertNegativeInfinity(final double value) {
Assertions.assertTrue(value < 0, msg);
}
- /** Asserts that the given Runnable throws an exception of the given type.
- * @param r the Runnable instance
- * @param exceptionType the expected exception type
- */
- public static void assertThrows(final Runnable r, final Class<?> exceptionType) {
- assertThrows(r, exceptionType, (String) null);
- }
-
- /** Asserts that the given Runnable throws an exception of the given type. If
+ /** Asserts that the given Executable throws an exception of the given type. If
* {@code message} is not null, the exception message is asserted to equal the
* given value.
- * @param r the Runnable instance
+ * @param executable the Executable instance
* @param exceptionType the expected exception type
* @param message the expected exception message; ignored if null
*/
- public static void assertThrows(final Runnable r, final Class<?> exceptionType, final String message) {
- try {
- r.run();
- Assertions.fail("Operation should have thrown an exception");
- } catch (final Exception exc) {
- final Class<?> actualType = exc.getClass();
-
- Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
- "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
-
- if (message != null) {
- Assertions.assertEquals(message, exc.getMessage());
- }
- }
+ public static <T extends Throwable> void assertThrowsWithMessage(final Executable executable, final Class<T> exceptionType, final String message) {
+ Assertions.assertEquals(message, Assertions.assertThrows(exceptionType, executable).getMessage());
}
- /** Asserts that the given Runnable throws an exception of the given type. If
+ /** Asserts that the given Executable throws an exception of the given type. If
* {@code pattern} is not null, the exception message is asserted to match the
* given regex.
- * @param r the Runnable instance
+ * @param executable the Executable instance
* @param exceptionType the expected exception type
* @param pattern regex pattern to match; ignored if null
Review comment:
Same as above: the comments here state that the pattern argument is ignored if null, which is no longer correct.
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -50,61 +51,28 @@ public static void assertNegativeInfinity(final double value) {
Assertions.assertTrue(value < 0, msg);
}
- /** Asserts that the given Runnable throws an exception of the given type.
- * @param r the Runnable instance
- * @param exceptionType the expected exception type
- */
- public static void assertThrows(final Runnable r, final Class<?> exceptionType) {
- assertThrows(r, exceptionType, (String) null);
- }
-
- /** Asserts that the given Runnable throws an exception of the given type. If
+ /** Asserts that the given Executable throws an exception of the given type. If
* {@code message} is not null, the exception message is asserted to equal the
* given value.
- * @param r the Runnable instance
+ * @param executable the Executable instance
* @param exceptionType the expected exception type
* @param message the expected exception message; ignored if null
Review comment:
The documentation here still includes parts about the expected message being ignored if null, which is no longer correct.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-geometry] singhbaljit commented on a change in pull request #121: [GEOMETRY-105] - Replace the try-catch pattern with assertThrows()
Posted by GitBox <gi...@apache.org>.
singhbaljit commented on a change in pull request #121:
URL: https://github.com/apache/commons-geometry/pull/121#discussion_r544480779
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -50,35 +50,17 @@ public static void assertNegativeInfinity(final double value) {
Assertions.assertTrue(value < 0, msg);
}
- /** Asserts that the given Runnable throws an exception of the given type.
- * @param r the Runnable instance
- * @param exceptionType the expected exception type
- */
- public static void assertThrows(final Runnable r, final Class<?> exceptionType) {
- assertThrows(r, exceptionType, (String) null);
- }
-
/** Asserts that the given Runnable throws an exception of the given type. If
* {@code message} is not null, the exception message is asserted to equal the
* given value.
* @param r the Runnable instance
* @param exceptionType the expected exception type
* @param message the expected exception message; ignored if null
*/
- public static void assertThrows(final Runnable r, final Class<?> exceptionType, final String message) {
- try {
- r.run();
- Assertions.fail("Operation should have thrown an exception");
- } catch (final Exception exc) {
- final Class<?> actualType = exc.getClass();
-
- Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
- "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
-
- if (message != null) {
- Assertions.assertEquals(message, exc.getMessage());
- }
- }
+ public static <T extends Throwable> void assertThrowsWithMessage(final Runnable r, final Class<T> exceptionType, final String message) {
Review comment:
It is safe to replace `Runnable` with `org.junit.jupiter.api.function.Executable`. Lambda signature is equivalent, so this is a non-breaking change. You can use pass the along the `r` instead of creating another lambda instance as `r::run`.
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -50,35 +50,17 @@ public static void assertNegativeInfinity(final double value) {
Assertions.assertTrue(value < 0, msg);
}
- /** Asserts that the given Runnable throws an exception of the given type.
- * @param r the Runnable instance
- * @param exceptionType the expected exception type
- */
- public static void assertThrows(final Runnable r, final Class<?> exceptionType) {
- assertThrows(r, exceptionType, (String) null);
- }
-
/** Asserts that the given Runnable throws an exception of the given type. If
* {@code message} is not null, the exception message is asserted to equal the
* given value.
* @param r the Runnable instance
* @param exceptionType the expected exception type
* @param message the expected exception message; ignored if null
*/
- public static void assertThrows(final Runnable r, final Class<?> exceptionType, final String message) {
- try {
- r.run();
- Assertions.fail("Operation should have thrown an exception");
- } catch (final Exception exc) {
- final Class<?> actualType = exc.getClass();
-
- Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
- "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
-
- if (message != null) {
- Assertions.assertEquals(message, exc.getMessage());
- }
- }
+ public static <T extends Throwable> void assertThrowsWithMessage(final Runnable r, final Class<T> exceptionType, final String message) {
+ final Throwable exc = Assertions.assertThrows(exceptionType, r::run);
+ Assertions.assertNotNull(message);
Review comment:
This should not be asserted. `null` is a valid expected message.
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -88,23 +70,15 @@ public static void assertThrows(final Runnable r, final Class<?> exceptionType,
* @param exceptionType the expected exception type
* @param pattern regex pattern to match; ignored if null
*/
- public static void assertThrows(final Runnable r, final Class<?> exceptionType, final Pattern pattern) {
- try {
- r.run();
- Assertions.fail("Operation should have thrown an exception");
- } catch (final Exception exc) {
- final Class<?> actualType = exc.getClass();
-
- Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
- "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
-
- if (pattern != null) {
- final String message = exc.getMessage();
-
- final String err = "Expected exception message to match /" + pattern + "/ but was [" + message + "]";
- Assertions.assertTrue(pattern.matcher(message).matches(), err);
- }
- }
+ public static <T extends Throwable> void assertThrowsWithMessage(final Runnable r, final Class<T> exceptionType, final Pattern pattern) {
+ final Throwable exc = Assertions.assertThrows(exceptionType, r::run);
+ Assertions.assertNotNull(pattern);
+ final Class<?> actualType = exc.getClass();
+ Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
+ "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
+ final String message = exc.getMessage();
+ final String err = "Expected exception message to match /" + pattern + "/ but was [" + message + "]";
+ Assertions.assertTrue(pattern.matcher(message).matches(), err);
Review comment:
Simplified version (style choice):
```java
Assertions.assertNotNull(pattern);
final String message = Assertions.assertThrows(exceptionType, r).getMessage();
Assertions.assertTrue(pattern.matcher(message).matches(),
"Expected exception message to match /" + pattern + "/ but was [" + message + "]");
```
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -50,35 +50,17 @@ public static void assertNegativeInfinity(final double value) {
Assertions.assertTrue(value < 0, msg);
}
- /** Asserts that the given Runnable throws an exception of the given type.
- * @param r the Runnable instance
- * @param exceptionType the expected exception type
- */
- public static void assertThrows(final Runnable r, final Class<?> exceptionType) {
- assertThrows(r, exceptionType, (String) null);
- }
-
/** Asserts that the given Runnable throws an exception of the given type. If
* {@code message} is not null, the exception message is asserted to equal the
* given value.
* @param r the Runnable instance
* @param exceptionType the expected exception type
* @param message the expected exception message; ignored if null
*/
- public static void assertThrows(final Runnable r, final Class<?> exceptionType, final String message) {
- try {
- r.run();
- Assertions.fail("Operation should have thrown an exception");
- } catch (final Exception exc) {
- final Class<?> actualType = exc.getClass();
-
- Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
- "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
-
- if (message != null) {
- Assertions.assertEquals(message, exc.getMessage());
- }
- }
+ public static <T extends Throwable> void assertThrowsWithMessage(final Runnable r, final Class<T> exceptionType, final String message) {
Review comment:
This can be a one-liner (style choice):
```java
Assertions.assertEquals(message, Assertions.assertThrows(exceptionType, r).getMessage())
```
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -88,23 +70,15 @@ public static void assertThrows(final Runnable r, final Class<?> exceptionType,
* @param exceptionType the expected exception type
* @param pattern regex pattern to match; ignored if null
*/
- public static void assertThrows(final Runnable r, final Class<?> exceptionType, final Pattern pattern) {
- try {
- r.run();
- Assertions.fail("Operation should have thrown an exception");
- } catch (final Exception exc) {
- final Class<?> actualType = exc.getClass();
-
- Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
- "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
-
- if (pattern != null) {
- final String message = exc.getMessage();
-
- final String err = "Expected exception message to match /" + pattern + "/ but was [" + message + "]";
- Assertions.assertTrue(pattern.matcher(message).matches(), err);
- }
- }
+ public static <T extends Throwable> void assertThrowsWithMessage(final Runnable r, final Class<T> exceptionType, final Pattern pattern) {
+ final Throwable exc = Assertions.assertThrows(exceptionType, r::run);
+ Assertions.assertNotNull(pattern);
+ final Class<?> actualType = exc.getClass();
+ Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
Review comment:
Unnecessary assertion. This will always pass.
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -88,23 +70,15 @@ public static void assertThrows(final Runnable r, final Class<?> exceptionType,
* @param exceptionType the expected exception type
* @param pattern regex pattern to match; ignored if null
*/
- public static void assertThrows(final Runnable r, final Class<?> exceptionType, final Pattern pattern) {
- try {
- r.run();
- Assertions.fail("Operation should have thrown an exception");
- } catch (final Exception exc) {
- final Class<?> actualType = exc.getClass();
-
- Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
- "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
-
- if (pattern != null) {
- final String message = exc.getMessage();
-
- final String err = "Expected exception message to match /" + pattern + "/ but was [" + message + "]";
- Assertions.assertTrue(pattern.matcher(message).matches(), err);
- }
- }
+ public static <T extends Throwable> void assertThrowsWithMessage(final Runnable r, final Class<T> exceptionType, final Pattern pattern) {
Review comment:
Same comment for `Runnable`.
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/partitioning/AbstractConvexHyperplaneBoundedRegionTest.java
##########
@@ -33,6 +33,8 @@
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertThrows;
Review comment:
Static imports are fine IMO, but stylistically, the library tests are written `Assertions.assert*`. So, we should keep it consistent.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-geometry] arturobernalg commented on a change in pull request #121: [GEOMETRY-105] - Replace the try-catch pattern with assertThrows()
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #121:
URL: https://github.com/apache/commons-geometry/pull/121#discussion_r544828632
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -50,61 +51,29 @@ public static void assertNegativeInfinity(final double value) {
Assertions.assertTrue(value < 0, msg);
}
- /** Asserts that the given Runnable throws an exception of the given type.
- * @param r the Runnable instance
- * @param exceptionType the expected exception type
- */
- public static void assertThrows(final Runnable r, final Class<?> exceptionType) {
- assertThrows(r, exceptionType, (String) null);
- }
-
/** Asserts that the given Runnable throws an exception of the given type. If
* {@code message} is not null, the exception message is asserted to equal the
* given value.
- * @param r the Runnable instance
+ * @param executable the Executable instance
* @param exceptionType the expected exception type
* @param message the expected exception message; ignored if null
*/
- public static void assertThrows(final Runnable r, final Class<?> exceptionType, final String message) {
- try {
- r.run();
- Assertions.fail("Operation should have thrown an exception");
- } catch (final Exception exc) {
- final Class<?> actualType = exc.getClass();
-
- Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
- "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
-
- if (message != null) {
- Assertions.assertEquals(message, exc.getMessage());
- }
- }
+ public static <T extends Throwable> void assertThrowsWithMessage(final Executable executable, final Class<T> exceptionType, final String message) {
+ Assertions.assertEquals(message, Assertions.assertThrows(exceptionType, executable).getMessage());
}
/** Asserts that the given Runnable throws an exception of the given type. If
* {@code pattern} is not null, the exception message is asserted to match the
* given regex.
- * @param r the Runnable instance
+ * @param executable the Executable instance
* @param exceptionType the expected exception type
* @param pattern regex pattern to match; ignored if null
*/
- public static void assertThrows(final Runnable r, final Class<?> exceptionType, final Pattern pattern) {
- try {
- r.run();
- Assertions.fail("Operation should have thrown an exception");
- } catch (final Exception exc) {
- final Class<?> actualType = exc.getClass();
-
- Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
- "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
-
- if (pattern != null) {
- final String message = exc.getMessage();
-
- final String err = "Expected exception message to match /" + pattern + "/ but was [" + message + "]";
- Assertions.assertTrue(pattern.matcher(message).matches(), err);
- }
- }
+ public static <T extends Throwable> void assertThrowsWithMessage(final Executable executable, final Class<T> exceptionType, final Pattern pattern) {
+ Assertions.assertNotNull(pattern);
Review comment:
changed
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -50,61 +51,29 @@ public static void assertNegativeInfinity(final double value) {
Assertions.assertTrue(value < 0, msg);
}
- /** Asserts that the given Runnable throws an exception of the given type.
- * @param r the Runnable instance
- * @param exceptionType the expected exception type
- */
- public static void assertThrows(final Runnable r, final Class<?> exceptionType) {
- assertThrows(r, exceptionType, (String) null);
- }
-
/** Asserts that the given Runnable throws an exception of the given type. If
* {@code message} is not null, the exception message is asserted to equal the
* given value.
- * @param r the Runnable instance
+ * @param executable the Executable instance
* @param exceptionType the expected exception type
* @param message the expected exception message; ignored if null
*/
- public static void assertThrows(final Runnable r, final Class<?> exceptionType, final String message) {
- try {
- r.run();
- Assertions.fail("Operation should have thrown an exception");
- } catch (final Exception exc) {
- final Class<?> actualType = exc.getClass();
-
- Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
- "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
-
- if (message != null) {
- Assertions.assertEquals(message, exc.getMessage());
- }
- }
+ public static <T extends Throwable> void assertThrowsWithMessage(final Executable executable, final Class<T> exceptionType, final String message) {
Review comment:
changed
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-geometry] arturobernalg commented on a change in pull request #121: [GEOMETRY-105] - Replace the try-catch pattern with assertThrows()
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #121:
URL: https://github.com/apache/commons-geometry/pull/121#discussion_r544599428
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -50,35 +50,17 @@ public static void assertNegativeInfinity(final double value) {
Assertions.assertTrue(value < 0, msg);
}
- /** Asserts that the given Runnable throws an exception of the given type.
- * @param r the Runnable instance
- * @param exceptionType the expected exception type
- */
- public static void assertThrows(final Runnable r, final Class<?> exceptionType) {
- assertThrows(r, exceptionType, (String) null);
- }
-
/** Asserts that the given Runnable throws an exception of the given type. If
* {@code message} is not null, the exception message is asserted to equal the
* given value.
* @param r the Runnable instance
* @param exceptionType the expected exception type
* @param message the expected exception message; ignored if null
*/
- public static void assertThrows(final Runnable r, final Class<?> exceptionType, final String message) {
- try {
- r.run();
- Assertions.fail("Operation should have thrown an exception");
- } catch (final Exception exc) {
- final Class<?> actualType = exc.getClass();
-
- Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
- "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
-
- if (message != null) {
- Assertions.assertEquals(message, exc.getMessage());
- }
- }
+ public static <T extends Throwable> void assertThrowsWithMessage(final Runnable r, final Class<T> exceptionType, final String message) {
+ final Throwable exc = Assertions.assertThrows(exceptionType, r::run);
+ Assertions.assertNotNull(message);
Review comment:
changed
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -50,35 +50,17 @@ public static void assertNegativeInfinity(final double value) {
Assertions.assertTrue(value < 0, msg);
}
- /** Asserts that the given Runnable throws an exception of the given type.
- * @param r the Runnable instance
- * @param exceptionType the expected exception type
- */
- public static void assertThrows(final Runnable r, final Class<?> exceptionType) {
- assertThrows(r, exceptionType, (String) null);
- }
-
/** Asserts that the given Runnable throws an exception of the given type. If
* {@code message} is not null, the exception message is asserted to equal the
* given value.
* @param r the Runnable instance
* @param exceptionType the expected exception type
* @param message the expected exception message; ignored if null
*/
- public static void assertThrows(final Runnable r, final Class<?> exceptionType, final String message) {
- try {
- r.run();
- Assertions.fail("Operation should have thrown an exception");
- } catch (final Exception exc) {
- final Class<?> actualType = exc.getClass();
-
- Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
- "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
-
- if (message != null) {
- Assertions.assertEquals(message, exc.getMessage());
- }
- }
+ public static <T extends Throwable> void assertThrowsWithMessage(final Runnable r, final Class<T> exceptionType, final String message) {
Review comment:
changed
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-geometry] asfgit merged pull request #121: [GEOMETRY-105] - Replace the try-catch pattern with assertThrows()
Posted by GitBox <gi...@apache.org>.
asfgit merged pull request #121:
URL: https://github.com/apache/commons-geometry/pull/121
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-geometry] darkma773r commented on a change in pull request #121: [GEOMETRY-105] - Replace the try-catch pattern with assertThrows()
Posted by GitBox <gi...@apache.org>.
darkma773r commented on a change in pull request #121:
URL: https://github.com/apache/commons-geometry/pull/121#discussion_r544764724
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/partitioning/AbstractConvexHyperplaneBoundedRegionTest.java
##########
@@ -33,6 +33,8 @@
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertThrows;
Review comment:
Agreed. For consistency, we should use the qualified `Assertions.assertThrows(...)` instead of just `assertThrows(...)`. This convention is used in commons-math and commons-numbers as well.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-geometry] arturobernalg commented on a change in pull request #121: [GEOMETRY-105] - Replace the try-catch pattern with assertThrows()
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #121:
URL: https://github.com/apache/commons-geometry/pull/121#discussion_r545581850
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -50,61 +51,28 @@ public static void assertNegativeInfinity(final double value) {
Assertions.assertTrue(value < 0, msg);
}
- /** Asserts that the given Runnable throws an exception of the given type.
- * @param r the Runnable instance
- * @param exceptionType the expected exception type
- */
- public static void assertThrows(final Runnable r, final Class<?> exceptionType) {
- assertThrows(r, exceptionType, (String) null);
- }
-
- /** Asserts that the given Runnable throws an exception of the given type. If
+ /** Asserts that the given Executable throws an exception of the given type. If
* {@code message} is not null, the exception message is asserted to equal the
* given value.
- * @param r the Runnable instance
+ * @param executable the Executable instance
* @param exceptionType the expected exception type
* @param message the expected exception message; ignored if null
Review comment:
true. mi mistake changed
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -50,61 +51,28 @@ public static void assertNegativeInfinity(final double value) {
Assertions.assertTrue(value < 0, msg);
}
- /** Asserts that the given Runnable throws an exception of the given type.
- * @param r the Runnable instance
- * @param exceptionType the expected exception type
- */
- public static void assertThrows(final Runnable r, final Class<?> exceptionType) {
- assertThrows(r, exceptionType, (String) null);
- }
-
- /** Asserts that the given Runnable throws an exception of the given type. If
+ /** Asserts that the given Executable throws an exception of the given type. If
* {@code message} is not null, the exception message is asserted to equal the
* given value.
- * @param r the Runnable instance
+ * @param executable the Executable instance
* @param exceptionType the expected exception type
* @param message the expected exception message; ignored if null
*/
- public static void assertThrows(final Runnable r, final Class<?> exceptionType, final String message) {
- try {
- r.run();
- Assertions.fail("Operation should have thrown an exception");
- } catch (final Exception exc) {
- final Class<?> actualType = exc.getClass();
-
- Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
- "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
-
- if (message != null) {
- Assertions.assertEquals(message, exc.getMessage());
- }
- }
+ public static <T extends Throwable> void assertThrowsWithMessage(final Executable executable, final Class<T> exceptionType, final String message) {
+ Assertions.assertEquals(message, Assertions.assertThrows(exceptionType, executable).getMessage());
}
- /** Asserts that the given Runnable throws an exception of the given type. If
+ /** Asserts that the given Executable throws an exception of the given type. If
* {@code pattern} is not null, the exception message is asserted to match the
* given regex.
- * @param r the Runnable instance
+ * @param executable the Executable instance
* @param exceptionType the expected exception type
* @param pattern regex pattern to match; ignored if null
Review comment:
idem
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-geometry] darkma773r commented on a change in pull request #121: [GEOMETRY-105] - Replace the try-catch pattern with assertThrows()
Posted by GitBox <gi...@apache.org>.
darkma773r commented on a change in pull request #121:
URL: https://github.com/apache/commons-geometry/pull/121#discussion_r544768403
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -50,61 +51,29 @@ public static void assertNegativeInfinity(final double value) {
Assertions.assertTrue(value < 0, msg);
}
- /** Asserts that the given Runnable throws an exception of the given type.
- * @param r the Runnable instance
- * @param exceptionType the expected exception type
- */
- public static void assertThrows(final Runnable r, final Class<?> exceptionType) {
- assertThrows(r, exceptionType, (String) null);
- }
-
/** Asserts that the given Runnable throws an exception of the given type. If
* {@code message} is not null, the exception message is asserted to equal the
* given value.
- * @param r the Runnable instance
+ * @param executable the Executable instance
* @param exceptionType the expected exception type
* @param message the expected exception message; ignored if null
*/
- public static void assertThrows(final Runnable r, final Class<?> exceptionType, final String message) {
- try {
- r.run();
- Assertions.fail("Operation should have thrown an exception");
- } catch (final Exception exc) {
- final Class<?> actualType = exc.getClass();
-
- Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
- "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
-
- if (message != null) {
- Assertions.assertEquals(message, exc.getMessage());
- }
- }
+ public static <T extends Throwable> void assertThrowsWithMessage(final Executable executable, final Class<T> exceptionType, final String message) {
Review comment:
Javadoc comment needs to be updated to replace `Runnable` with `Executable` and to remove the parts that say the message assertion is skipped if `message` is null.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-geometry] arturobernalg commented on pull request #121: [GEOMETRY-105] - Replace the try-catch pattern with assertThrows()
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on pull request #121:
URL: https://github.com/apache/commons-geometry/pull/121#issuecomment-746957655
Hi @singhbaljit
I have made all the changes except the import. IMO i't not a good practice use wildcard. could lead to conflicts or errors
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-geometry] arturobernalg commented on a change in pull request #121: [GEOMETRY-105] - Replace the try-catch pattern with assertThrows()
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #121:
URL: https://github.com/apache/commons-geometry/pull/121#discussion_r545581960
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/precision/EpsilonDoublePrecisionContextTest.java
##########
@@ -20,6 +20,7 @@
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
+
Review comment:
my mistake. changed
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -50,61 +51,28 @@ public static void assertNegativeInfinity(final double value) {
Assertions.assertTrue(value < 0, msg);
}
- /** Asserts that the given Runnable throws an exception of the given type.
- * @param r the Runnable instance
- * @param exceptionType the expected exception type
- */
- public static void assertThrows(final Runnable r, final Class<?> exceptionType) {
- assertThrows(r, exceptionType, (String) null);
- }
-
- /** Asserts that the given Runnable throws an exception of the given type. If
+ /** Asserts that the given Executable throws an exception of the given type. If
* {@code message} is not null, the exception message is asserted to equal the
* given value.
- * @param r the Runnable instance
+ * @param executable the Executable instance
* @param exceptionType the expected exception type
* @param message the expected exception message; ignored if null
Review comment:
true. my mistake changed
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-geometry] singhbaljit commented on a change in pull request #121: [GEOMETRY-105] - Replace the try-catch pattern with assertThrows()
Posted by GitBox <gi...@apache.org>.
singhbaljit commented on a change in pull request #121:
URL: https://github.com/apache/commons-geometry/pull/121#discussion_r544480779
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -50,35 +50,17 @@ public static void assertNegativeInfinity(final double value) {
Assertions.assertTrue(value < 0, msg);
}
- /** Asserts that the given Runnable throws an exception of the given type.
- * @param r the Runnable instance
- * @param exceptionType the expected exception type
- */
- public static void assertThrows(final Runnable r, final Class<?> exceptionType) {
- assertThrows(r, exceptionType, (String) null);
- }
-
/** Asserts that the given Runnable throws an exception of the given type. If
* {@code message} is not null, the exception message is asserted to equal the
* given value.
* @param r the Runnable instance
* @param exceptionType the expected exception type
* @param message the expected exception message; ignored if null
*/
- public static void assertThrows(final Runnable r, final Class<?> exceptionType, final String message) {
- try {
- r.run();
- Assertions.fail("Operation should have thrown an exception");
- } catch (final Exception exc) {
- final Class<?> actualType = exc.getClass();
-
- Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
- "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
-
- if (message != null) {
- Assertions.assertEquals(message, exc.getMessage());
- }
- }
+ public static <T extends Throwable> void assertThrowsWithMessage(final Runnable r, final Class<T> exceptionType, final String message) {
Review comment:
It is safe to replace `Runnable` with `org.junit.jupiter.api.function.Executable`. Lambda signature is equivalent, so this is a non-breaking change. You can just pass the along the `r` instead of creating another lambda instance as `r::run`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-geometry] arturobernalg commented on a change in pull request #121: [GEOMETRY-105] - Replace the try-catch pattern with assertThrows()
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #121:
URL: https://github.com/apache/commons-geometry/pull/121#discussion_r544599521
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -88,23 +70,15 @@ public static void assertThrows(final Runnable r, final Class<?> exceptionType,
* @param exceptionType the expected exception type
* @param pattern regex pattern to match; ignored if null
*/
- public static void assertThrows(final Runnable r, final Class<?> exceptionType, final Pattern pattern) {
- try {
- r.run();
- Assertions.fail("Operation should have thrown an exception");
- } catch (final Exception exc) {
- final Class<?> actualType = exc.getClass();
-
- Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
- "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
-
- if (pattern != null) {
- final String message = exc.getMessage();
-
- final String err = "Expected exception message to match /" + pattern + "/ but was [" + message + "]";
- Assertions.assertTrue(pattern.matcher(message).matches(), err);
- }
- }
+ public static <T extends Throwable> void assertThrowsWithMessage(final Runnable r, final Class<T> exceptionType, final Pattern pattern) {
+ final Throwable exc = Assertions.assertThrows(exceptionType, r::run);
+ Assertions.assertNotNull(pattern);
+ final Class<?> actualType = exc.getClass();
+ Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
Review comment:
changed
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -88,23 +70,15 @@ public static void assertThrows(final Runnable r, final Class<?> exceptionType,
* @param exceptionType the expected exception type
* @param pattern regex pattern to match; ignored if null
*/
- public static void assertThrows(final Runnable r, final Class<?> exceptionType, final Pattern pattern) {
- try {
- r.run();
- Assertions.fail("Operation should have thrown an exception");
- } catch (final Exception exc) {
- final Class<?> actualType = exc.getClass();
-
- Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
- "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
-
- if (pattern != null) {
- final String message = exc.getMessage();
-
- final String err = "Expected exception message to match /" + pattern + "/ but was [" + message + "]";
- Assertions.assertTrue(pattern.matcher(message).matches(), err);
- }
- }
+ public static <T extends Throwable> void assertThrowsWithMessage(final Runnable r, final Class<T> exceptionType, final Pattern pattern) {
+ final Throwable exc = Assertions.assertThrows(exceptionType, r::run);
+ Assertions.assertNotNull(pattern);
+ final Class<?> actualType = exc.getClass();
+ Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
+ "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
+ final String message = exc.getMessage();
+ final String err = "Expected exception message to match /" + pattern + "/ but was [" + message + "]";
+ Assertions.assertTrue(pattern.matcher(message).matches(), err);
Review comment:
changed
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-geometry] darkma773r commented on a change in pull request #121: [GEOMETRY-105] - Replace the try-catch pattern with assertThrows()
Posted by GitBox <gi...@apache.org>.
darkma773r commented on a change in pull request #121:
URL: https://github.com/apache/commons-geometry/pull/121#discussion_r544767499
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -50,61 +51,29 @@ public static void assertNegativeInfinity(final double value) {
Assertions.assertTrue(value < 0, msg);
}
- /** Asserts that the given Runnable throws an exception of the given type.
- * @param r the Runnable instance
- * @param exceptionType the expected exception type
- */
- public static void assertThrows(final Runnable r, final Class<?> exceptionType) {
- assertThrows(r, exceptionType, (String) null);
- }
-
/** Asserts that the given Runnable throws an exception of the given type. If
* {@code message} is not null, the exception message is asserted to equal the
* given value.
- * @param r the Runnable instance
+ * @param executable the Executable instance
* @param exceptionType the expected exception type
* @param message the expected exception message; ignored if null
*/
- public static void assertThrows(final Runnable r, final Class<?> exceptionType, final String message) {
- try {
- r.run();
- Assertions.fail("Operation should have thrown an exception");
- } catch (final Exception exc) {
- final Class<?> actualType = exc.getClass();
-
- Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
- "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
-
- if (message != null) {
- Assertions.assertEquals(message, exc.getMessage());
- }
- }
+ public static <T extends Throwable> void assertThrowsWithMessage(final Executable executable, final Class<T> exceptionType, final String message) {
+ Assertions.assertEquals(message, Assertions.assertThrows(exceptionType, executable).getMessage());
}
/** Asserts that the given Runnable throws an exception of the given type. If
* {@code pattern} is not null, the exception message is asserted to match the
* given regex.
- * @param r the Runnable instance
+ * @param executable the Executable instance
* @param exceptionType the expected exception type
* @param pattern regex pattern to match; ignored if null
*/
- public static void assertThrows(final Runnable r, final Class<?> exceptionType, final Pattern pattern) {
- try {
- r.run();
- Assertions.fail("Operation should have thrown an exception");
- } catch (final Exception exc) {
- final Class<?> actualType = exc.getClass();
-
- Assertions.assertTrue(exceptionType.isAssignableFrom(actualType),
- "Expected exception of type " + exceptionType.getName() + " but was " + actualType.getName());
-
- if (pattern != null) {
- final String message = exc.getMessage();
-
- final String err = "Expected exception message to match /" + pattern + "/ but was [" + message + "]";
- Assertions.assertTrue(pattern.matcher(message).matches(), err);
- }
- }
+ public static <T extends Throwable> void assertThrowsWithMessage(final Executable executable, final Class<T> exceptionType, final Pattern pattern) {
+ Assertions.assertNotNull(pattern);
Review comment:
Don't use a test assertion here. If `pattern` is null, that indicates a usage error and not a test failure. Either replace with `Objects.requireNonNull()` or just remove this line. The Javadoc comment also needs to be updated to replace `Runnable` with `Executable` and to indicate that `pattern` is required.
----------------------------------------------------------------
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:
users@infra.apache.org