pzygielo commented on code in PR #232:
URL: https://github.com/apache/maven-scm/pull/232#discussion_r1882341152
##########
maven-scm-plugin/src/test/java/org/apache/maven/scm/plugin/ChangeLogMojoTest.java:
##########
@@ -103,7 +103,7 @@ public void testChangeLogWithBadUserDateFormat() throws
Exception {
fail("mojo execution must fail.");
} catch (MojoExecutionException e) {
- assertTrue(true);
+ assertNotNull(e.getMessage());
Review Comment:
Proposed change introduces new constraint for the exception message. It
wasn't there, wasn't needed then, and isn't needed now.
I think the always true assertion was added to satisfy checkstyle to avoid
empty block.
If this PR gets merged, refactoring to come next, would need to consider
that. Not only that the exception is thrown (what is currently checked) but
also that the message is not null.
IMO, it's unnecessary and it would be better to migrate to `assertThrows`
only.
`assertTrue(true)` could then be eliminated.
`assertNotNull(...)` couldn't.
--
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: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
