[
https://issues.apache.org/jira/browse/ARTEMIS-5283?focusedWorklogId=955273&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-955273
]
ASF GitHub Bot logged work on ARTEMIS-5283:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 03/Feb/25 18:13
Start Date: 03/Feb/25 18:13
Worklog Time Spent: 10m
Work Description: 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
Issue Time Tracking
-------------------
Worklog Id: (was: 955273)
Time Spent: 50m (was: 40m)
> Tune up JUnit tests
> -------------------
>
> Key: ARTEMIS-5283
> URL: https://issues.apache.org/jira/browse/ARTEMIS-5283
> Project: ActiveMQ Artemis
> Issue Type: Task
> Reporter: Justin Bertram
> Assignee: Justin Bertram
> Priority: Major
> Labels: pull-request-available
> Time Spent: 50m
> Remaining Estimate: 0h
>
> There are a number of categorical improvements & fixes that can be made to
> the JUnit tests throughout the code-base. This Jira will function as a parent
> to all the sub-tasks for this endeavor.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
