gemmellr commented on code in PR #5485:
URL: https://github.com/apache/activemq-artemis/pull/5485#discussion_r1939757225
##########
artemis-commons/src/test/java/org/apache/activemq/artemis/utils/ByteUtilTest.java:
##########
@@ -142,7 +142,7 @@ public void testTextBytesToLongBytesNegative() {
ByteUtil.convertTextBytes("x");
fail();
} catch (Exception e) {
- assertTrue(e instanceof IllegalArgumentException);
+ assertInstanceOf(IllegalArgumentException.class, e);
Review Comment:
Whole test could be replaced with the even simpler:
```
assertThrows(IllegalArgumentException.class, () -> {
ByteUtil.convertTextBytes("x");
});
```
##########
artemis-protocols/artemis-amqp-protocol/src/test/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessageTest.java:
##########
@@ -2947,7 +2946,7 @@ private boolean isEquals(Properties left, Properties
right) {
try {
assertEquals(left.getAbsoluteExpiryTime(),
right.getAbsoluteExpiryTime());
assertEquals(left.getContentEncoding(),
right.getAbsoluteExpiryTime());
- assertEquals(left.getContentType(), right.getContentType());
+ assertSame(left.getContentType(), right.getContentType());
Review Comment:
Would leave as assertEquals like others.
##########
artemis-server/src/test/java/org/apache/activemq/artemis/tests/util/SimpleStringTest.java:
##########
@@ -208,14 +209,14 @@ public void testCharSequence() throws Exception {
@Test
public void testEquals() throws Exception {
- assertFalse(createSimpleString("abcdef").equals(new Object()));
+ assertNotEquals(new Object(), createSimpleString("abcdef"));
- assertFalse(createSimpleString("abcef").equals(null));
+ assertNotEquals(null, createSimpleString("abcef"));
Review Comment:
This breaks the test as like others.
Similarly would leave these _equals_ tests all as assertTrue/False (inc
changes above and below)
##########
tests/db-tests/src/test/java/org/apache/activemq/artemis/tests/db/paging/PagingTest.java:
##########
@@ -6416,7 +6416,7 @@ public void testLivePageCacheEvicted() throws Throwable {
for (int i = 0; i < num; i++) {
msgReceivedCons = cons.receive(5000);
assertNotNull(msgReceivedCons);
- assertTrue(msgReceivedCons.getIntProperty("index") == i);
+ assertEquals((int) msgReceivedCons.getIntProperty("index"), i);
Review Comment:
the args order is reversed
##########
artemis-protocols/artemis-amqp-protocol/src/test/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessageTest.java:
##########
@@ -3090,7 +3089,7 @@ private boolean isEquals(Section left, Section right) {
return false;
}
- assertTrue(left.getClass().equals(right.getClass()));
+ assertSame(left.getClass(), right.getClass());
Review Comment:
Would leave as assertEquals like others.
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/security/RoleTest.java:
##########
@@ -102,30 +104,30 @@ public void testEqualsAndHashcode() throws Exception {
Role roleWithDifferentView = new Role("testEquals", true, true, true,
false, false, false, false, false, false, false, true, false);
Role roleWithDifferentUpdate = new Role("testEquals", true, true, true,
false, false, false, false, false, false, false, false, true);
- assertTrue(role.equals(role));
+ assertEquals(role, role);
- assertTrue(role.equals(sameRole));
- assertTrue(role.hashCode() == sameRole.hashCode());
+ assertEquals(role, sameRole);
+ assertEquals(role.hashCode(), sameRole.hashCode());
- assertFalse(role.equals(roleWithDifferentName));
- assertFalse(role.hashCode() == roleWithDifferentName.hashCode());
+ assertNotEquals(role, roleWithDifferentName);
+ assertNotEquals(role.hashCode(), roleWithDifferentName.hashCode());
- assertFalse(role.equals(roleWithDifferentRead));
- assertFalse(role.hashCode() == roleWithDifferentRead.hashCode());
+ assertNotEquals(role, roleWithDifferentRead);
+ assertNotEquals(role.hashCode(), roleWithDifferentRead.hashCode());
- assertFalse(role.equals(roleWithDifferentWrite));
- assertFalse(role.hashCode() == roleWithDifferentWrite.hashCode());
+ assertNotEquals(role, roleWithDifferentWrite);
+ assertNotEquals(role.hashCode(), roleWithDifferentWrite.hashCode());
- assertFalse(role.equals(roleWithDifferentCreate));
- assertFalse(role.hashCode() == roleWithDifferentCreate.hashCode());
+ assertNotEquals(role, roleWithDifferentCreate);
+ assertNotEquals(role.hashCode(), roleWithDifferentCreate.hashCode());
- assertFalse(role.equals(roleWithDifferentView));
- assertFalse(role.hashCode() == roleWithDifferentView.hashCode());
+ assertNotEquals(role, roleWithDifferentView);
+ assertNotEquals(role.hashCode(), roleWithDifferentView.hashCode());
- assertFalse(role.equals(roleWithDifferentUpdate));
- assertFalse(role.hashCode() == roleWithDifferentUpdate.hashCode());
+ assertNotEquals(role, roleWithDifferentUpdate);
+ assertNotEquals(role.hashCode(), roleWithDifferentUpdate.hashCode());
- assertFalse(role.equals(null));
+ assertNotEquals(null, role);
Review Comment:
This shouldnt be changed like this, it breaks the test intention.
The test is verifying behaviour of the equals method of the Role object with
a null arg. By calling the assertNotEquals with null first, JUnit will actually
entirely escape the check and just do a null comparison itself, returning false
from its internal comparison whilst never having called the _equals_ under test.
Only this one is actually _wrong_, but personally I would actually change
_all_ the calls using/testing equals back the way they were, so that the test
is plainly validating the expected boolean returns directly itself. Comparing
the hashcodes with assert[Not]Equals is fine as those are already just numeric
returns.
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/security/jaas/RolePrincipalTest.java:
##########
@@ -55,10 +54,10 @@ public void testEquals() {
RolePrincipal p2 = new RolePrincipal("FOO");
RolePrincipal p3 = new RolePrincipal("BAR");
- assertTrue(p1.equals(p1));
- assertTrue(p1.equals(p2));
- assertFalse(p1.equals(null));
- assertFalse(p1.equals("FOO"));
- assertFalse(p1.equals(p3));
+ assertEquals(p1, p1);
+ assertEquals(p1, p2);
+ assertNotEquals(null, p1);
+ assertNotEquals("FOO", p1);
+ assertNotEquals(p1, p3);
Review Comment:
Similarly
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/version/impl/VersionImplTest.java:
##########
@@ -54,11 +53,11 @@ public void testEquals() throws Exception {
VersionImpl sameVersion = new VersionImpl("ACTIVEMQ", 2, 0, 1, 10, new
int[]{7, 8, 9, 10});
VersionImpl differentVersion = new VersionImpl("ACTIVEMQ", 2, 0, 1, 11,
new int[]{7, 8, 9, 10, 11});
- assertFalse(version.equals(new Object()));
+ assertNotEquals(new Object(), version);
- assertTrue(version.equals(version));
- assertTrue(version.equals(sameVersion));
- assertFalse(version.equals(differentVersion));
+ assertEquals(version, version);
+ assertEquals(version, sameVersion);
+ assertNotEquals(version, differentVersion);
Review Comment:
Similarly would leave these all (inc changes above + below) as
assertTrue/False (even though time none are breaking anything)
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/DuplicateDetectionTest.java:
##########
@@ -826,7 +826,7 @@ public void testRollbackThenSend() throws Exception {
message = consumer.receive(5000);
assertNotNull(message);
- assertTrue(message.getStringProperty("key").equals(dupID1.toString()));
+ assertEquals(message.getStringProperty("key"), dupID1.toString());
Review Comment:
The arg order is reversed
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/security/jaas/UserPrincipalTest.java:
##########
@@ -55,10 +54,10 @@ public void testEquals() {
UserPrincipal p2 = new UserPrincipal("FOO");
UserPrincipal p3 = new UserPrincipal("BAR");
- assertTrue(p1.equals(p1));
- assertTrue(p1.equals(p2));
- assertFalse(p1.equals(null));
- assertFalse(p1.equals("FOO"));
- assertFalse(p1.equals(p3));
+ assertEquals(p1, p1);
+ assertEquals(p1, p2);
+ assertNotEquals(null, p1);
+ assertNotEquals("FOO", p1);
+ assertNotEquals(p1, p3);
Review Comment:
Similarly
--
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: gitbox-unsubscr...@activemq.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@activemq.apache.org
For additional commands, e-mail: gitbox-h...@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
