gemmellr commented on code in PR #5485:
URL: https://github.com/apache/activemq-artemis/pull/5485#discussion_r1939099218
##########
artemis-cli/src/test/java/org/apache/activemq/artemis/cli/commands/messages/ConnectionAbstractTest.java:
##########
@@ -63,7 +64,7 @@ public void testDefaultSourceAcceptorNoArtemis() {
connectionAbstract.execute(new TestActionContext());
} catch (Exception e) {
e.printStackTrace();
- assertEquals(JMSException.class, e.getClass());
+ assertSame(JMSException.class, e.getClass());
Review Comment:
Personally not a fan of this, and dont really think it is an improvement
either. I wouldnt normally consider using assert[Not]Same comparing Class
arguments outside of a _Classloading_ test for example. I cant say I really
recall anyone else doing it that way either.
I'd say using assertEquals is far more typical in this use case (on
checking, there isnt a single instance of assertSame for this use case in the
Artemis codebase, or some others I work with) and I expect this change will
just result in us having both conventions as new test code continues to
introduce more assertEquals in future. I'd leave them as-is.
Looking at this whole commit, I wouldn't actually change any of them
personally (inc the non-classes args). None of them are _wrong_ afterwards, but
equally none of them are wrong as-is, since the tests aren't really interested
in checking object identity but rather just equality. It makes sense for them
to use assertEquals, and in these specific cases it ends up doing the same
thing anyway. I dont think its actually needed or beneficial to change any of
them.
##########
tests/integration-tests-isolated/src/test/java/org/apache/activemq/artemis/tests/integration/isolated/web/WebServerComponentTest.java:
##########
@@ -121,7 +121,7 @@ public void testRequestLog() throws Exception {
ch.writeAndFlush(request);
assertTrue(latch.await(5, TimeUnit.SECONDS));
assertEquals("12345", clientHandler.body.toString());
- assertEquals(clientHandler.body.toString(), "12345");
+ assertEquals("12345", clientHandler.body.toString());
Review Comment:
Assert on line above appears exactly the same, so this could perhaps be
removed (or changed, if it wasnt just a c&p error, but it seems likely thats
what it was)
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/config/impl/HAPolicyConfigurationTest.java:
##########
@@ -123,11 +123,11 @@ public void primaryOnlyTest2() throws Exception {
ScaleDownPolicy scaleDownPolicy =
primaryOnlyPolicy.getScaleDownPolicy();
assertNotNull(scaleDownPolicy);
assertFalse(scaleDownPolicy.isEnabled());
- assertEquals(scaleDownPolicy.getGroupName(), "boo!");
- assertEquals(scaleDownPolicy.getDiscoveryGroup(), null);
+ assertEquals("boo!", scaleDownPolicy.getGroupName());
+ assertEquals(null, scaleDownPolicy.getDiscoveryGroup());
Review Comment:
Should use assertNull
##########
artemis-commons/src/test/java/org/apache/activemq/artemis/utils/collections/ConcurrentLongHashMapTest.java:
##########
@@ -263,35 +263,35 @@ public void testHashConflictWithDeletion() {
int bucket2 =
ConcurrentLongHashMap.signSafeMod(ConcurrentLongHashMap.hash(key2), Buckets);
assertEquals(bucket1, bucket2);
- assertEquals(map.put(key1, "value-1"), null);
- assertEquals(map.put(key2, "value-2"), null);
- assertEquals(map.size(), 2);
+ assertEquals(null, map.put(key1, "value-1"));
+ assertEquals(null, map.put(key2, "value-2"));
+ assertEquals(2, map.size());
- assertEquals(map.remove(key1), "value-1");
- assertEquals(map.size(), 1);
+ assertEquals("value-1", map.remove(key1));
+ assertEquals(1, map.size());
- assertEquals(map.put(key1, "value-1-overwrite"), null);
- assertEquals(map.size(), 2);
+ assertEquals(null, map.put(key1, "value-1-overwrite"));
+ assertEquals(2, map.size());
- assertEquals(map.remove(key1), "value-1-overwrite");
- assertEquals(map.size(), 1);
+ assertEquals("value-1-overwrite", map.remove(key1));
+ assertEquals(1, map.size());
- assertEquals(map.put(key2, "value-2-overwrite"), "value-2");
- assertEquals(map.get(key2), "value-2-overwrite");
+ assertEquals("value-2", map.put(key2, "value-2-overwrite"));
+ assertEquals("value-2-overwrite", map.get(key2));
- assertEquals(map.size(), 1);
- assertEquals(map.remove(key2), "value-2-overwrite");
+ assertEquals(1, map.size());
+ assertEquals("value-2-overwrite", map.remove(key2));
assertTrue(map.isEmpty());
}
@Test
public void testPutIfAbsent() {
ConcurrentLongHashMap<String> map = new ConcurrentLongHashMap<>();
- assertEquals(map.putIfAbsent(1, "one"), null);
- assertEquals(map.get(1), "one");
+ assertEquals(null, map.putIfAbsent(1, "one"));
Review Comment:
Should use assertNull
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/persistence/XmlImportExportTest.java:
##########
@@ -1268,8 +1268,8 @@ public void testImportWrongRoutingType() throws Exception
{
m = consumer.receive(5000);
assertNotNull(m);
- assertEquals(m.getBodyBuffer().readString(), payload);
- assertEquals(m.getRoutingType(), null);
+ assertEquals(payload, m.getBodyBuffer().readString());
+ assertEquals(null, m.getRoutingType());
Review Comment:
Should use assertNull
##########
artemis-commons/src/test/java/org/apache/activemq/artemis/utils/collections/ConcurrentLongHashMapTest.java:
##########
@@ -263,35 +263,35 @@ public void testHashConflictWithDeletion() {
int bucket2 =
ConcurrentLongHashMap.signSafeMod(ConcurrentLongHashMap.hash(key2), Buckets);
assertEquals(bucket1, bucket2);
- assertEquals(map.put(key1, "value-1"), null);
- assertEquals(map.put(key2, "value-2"), null);
- assertEquals(map.size(), 2);
+ assertEquals(null, map.put(key1, "value-1"));
+ assertEquals(null, map.put(key2, "value-2"));
+ assertEquals(2, map.size());
- assertEquals(map.remove(key1), "value-1");
- assertEquals(map.size(), 1);
+ assertEquals("value-1", map.remove(key1));
+ assertEquals(1, map.size());
- assertEquals(map.put(key1, "value-1-overwrite"), null);
- assertEquals(map.size(), 2);
+ assertEquals(null, map.put(key1, "value-1-overwrite"));
Review Comment:
Should use assertNull
##########
tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/ra/ResourceAdapterTest.java:
##########
@@ -57,30 +57,30 @@ public void testDefaultConnectionFactory() throws Exception
{
ActiveMQResourceAdapter ra = new ActiveMQResourceAdapter();
ra.setConnectorClassName(InVMConnectorFactory.class.getName());
ActiveMQConnectionFactory factory =
ra.getDefaultActiveMQConnectionFactory();
- assertEquals(factory.getCallTimeout(),
ActiveMQClient.DEFAULT_CALL_TIMEOUT);
- assertEquals(factory.getClientFailureCheckPeriod(),
ActiveMQClient.DEFAULT_CLIENT_FAILURE_CHECK_PERIOD);
- assertEquals(factory.getClientID(), null);
- assertEquals(factory.getConnectionLoadBalancingPolicyClassName(),
ActiveMQClient.DEFAULT_CONNECTION_LOAD_BALANCING_POLICY_CLASS_NAME);
- assertEquals(factory.getConnectionTTL(),
ActiveMQClient.DEFAULT_CONNECTION_TTL);
- assertEquals(factory.getConsumerMaxRate(),
ActiveMQClient.DEFAULT_CONSUMER_MAX_RATE);
- assertEquals(factory.getConsumerWindowSize(),
ActiveMQClient.DEFAULT_CONSUMER_WINDOW_SIZE);
- assertEquals(factory.getDupsOKBatchSize(),
ActiveMQClient.DEFAULT_ACK_BATCH_SIZE);
- assertEquals(factory.getMinLargeMessageSize(),
ActiveMQClient.DEFAULT_MIN_LARGE_MESSAGE_SIZE);
- assertEquals(factory.getProducerMaxRate(),
ActiveMQClient.DEFAULT_PRODUCER_MAX_RATE);
- assertEquals(factory.getConfirmationWindowSize(),
ActiveMQClient.DEFAULT_CONFIRMATION_WINDOW_SIZE);
+ assertEquals(ActiveMQClient.DEFAULT_CALL_TIMEOUT,
factory.getCallTimeout());
+ assertEquals(ActiveMQClient.DEFAULT_CLIENT_FAILURE_CHECK_PERIOD,
factory.getClientFailureCheckPeriod());
+ assertEquals(null, factory.getClientID());
Review Comment:
Should use assertNull
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/config/impl/HAPolicyConfigurationTest.java:
##########
@@ -584,11 +584,11 @@ public void SharedStoreBackupTest2() throws Exception {
assertTrue(sharedStoreBackupPolicy.isRestartBackup());
ScaleDownPolicy scaleDownPolicy =
sharedStoreBackupPolicy.getScaleDownPolicy();
assertNotNull(scaleDownPolicy);
- assertEquals(scaleDownPolicy.getGroupName(), "boo!");
- assertEquals(scaleDownPolicy.getDiscoveryGroup(), null);
+ assertEquals("boo!", scaleDownPolicy.getGroupName());
+ assertEquals(null, scaleDownPolicy.getDiscoveryGroup());
Review Comment:
Should use assertNull
##########
artemis-protocols/artemis-amqp-protocol/src/test/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessageTest.java:
##########
@@ -1660,7 +1660,7 @@ public void testExtraProperty() {
decoded.putExtraBytesProperty(name, original);
ICoreMessage coreMessage = decoded.toCore();
- assertEquals(original, coreMessage.getBytesProperty(name));
+ assertArrayEquals(original, coreMessage.getBytesProperty(name));
Review Comment:
I think this one is deliberate, and changing it to assertArrayEquals would
actually change the intent of the test.
I think it is checking the original array is used. So Ironically, unlike all
the ones changed in the other commit, I do think _this_ one should use
assertSame (but could just stay with assertEquals and achieve the same).
(The later checks below this do use assertArrayEquals, because there are
encodes/decodes inbetween, and so it is expected they cant be the same array,
but should be equal)
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/config/impl/HAPolicyConfigurationTest.java:
##########
@@ -487,17 +487,17 @@ public void ReplicaTest2() throws Exception {
HAPolicy haPolicy = server.getHAPolicy();
assertTrue(haPolicy instanceof ReplicaPolicy);
ReplicaPolicy replicaPolicy = (ReplicaPolicy) haPolicy;
- assertEquals(replicaPolicy.getGroupName(), "tiddles");
- assertEquals(replicaPolicy.getMaxSavedReplicatedJournalsSize(), 22);
- assertEquals(replicaPolicy.getClusterName(), "33rrrrr");
+ assertEquals("tiddles", replicaPolicy.getGroupName());
+ assertEquals(22, replicaPolicy.getMaxSavedReplicatedJournalsSize());
+ assertEquals("33rrrrr", replicaPolicy.getClusterName());
assertFalse(replicaPolicy.isRestartBackup());
ScaleDownPolicy scaleDownPolicy = replicaPolicy.getScaleDownPolicy();
assertNotNull(scaleDownPolicy);
- assertEquals(scaleDownPolicy.getGroupName(), "boo!");
- assertEquals(scaleDownPolicy.getDiscoveryGroup(), null);
+ assertEquals("boo!", scaleDownPolicy.getGroupName());
+ assertEquals(null, scaleDownPolicy.getDiscoveryGroup());
Review Comment:
Should use assertNull
##########
artemis-commons/src/test/java/org/apache/activemq/artemis/utils/collections/ConcurrentLongHashMapTest.java:
##########
@@ -52,21 +52,21 @@ public void simpleInsertions() {
assertNull(map.put(2, "two"));
assertNull(map.put(3, "three"));
- assertEquals(map.size(), 3);
+ assertEquals(3, map.size());
- assertEquals(map.get(1), "one");
- assertEquals(map.size(), 3);
+ assertEquals("one", map.get(1));
+ assertEquals(3, map.size());
- assertEquals(map.remove(1), "one");
- assertEquals(map.size(), 2);
- assertEquals(map.get(1), null);
- assertEquals(map.get(5), null);
- assertEquals(map.size(), 2);
+ assertEquals("one", map.remove(1));
+ assertEquals(2, map.size());
+ assertEquals(null, map.get(1));
+ assertEquals(null, map.get(5));
Review Comment:
Should use assertNull
##########
tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/message/impl/MessagePropertyTest.java:
##########
@@ -109,7 +109,7 @@ private void receiveMessages() throws Exception {
assertNull(message.getIngressTimestamp());
assertTrue(message.containsProperty("null-value"));
- assertEquals(message.getObjectProperty("null-value"), null);
+ assertEquals(null, message.getObjectProperty("null-value"));
Review Comment:
Should use assertNull
##########
artemis-commons/src/test/java/org/apache/activemq/artemis/utils/collections/ConcurrentLongHashMapTest.java:
##########
@@ -263,35 +263,35 @@ public void testHashConflictWithDeletion() {
int bucket2 =
ConcurrentLongHashMap.signSafeMod(ConcurrentLongHashMap.hash(key2), Buckets);
assertEquals(bucket1, bucket2);
- assertEquals(map.put(key1, "value-1"), null);
- assertEquals(map.put(key2, "value-2"), null);
- assertEquals(map.size(), 2);
+ assertEquals(null, map.put(key1, "value-1"));
+ assertEquals(null, map.put(key2, "value-2"));
Review Comment:
Should use assertNull
##########
tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/ra/ResourceAdapterTest.java:
##########
@@ -97,30 +97,30 @@ public void testCreateConnectionFactoryNoOverrides() throws
Exception {
ActiveMQResourceAdapter ra = new ActiveMQResourceAdapter();
ra.setConnectorClassName(InVMConnectorFactory.class.getName());
ActiveMQConnectionFactory factory = ra.getConnectionFactory(new
ConnectionFactoryProperties());
- assertEquals(factory.getCallTimeout(),
ActiveMQClient.DEFAULT_CALL_TIMEOUT);
- assertEquals(factory.getClientFailureCheckPeriod(),
ActiveMQClient.DEFAULT_CLIENT_FAILURE_CHECK_PERIOD);
- assertEquals(factory.getClientID(), null);
- assertEquals(factory.getConnectionLoadBalancingPolicyClassName(),
ActiveMQClient.DEFAULT_CONNECTION_LOAD_BALANCING_POLICY_CLASS_NAME);
- assertEquals(factory.getConnectionTTL(),
ActiveMQClient.DEFAULT_CONNECTION_TTL);
- assertEquals(factory.getConsumerMaxRate(),
ActiveMQClient.DEFAULT_CONSUMER_MAX_RATE);
- assertEquals(factory.getConsumerWindowSize(),
ActiveMQClient.DEFAULT_CONSUMER_WINDOW_SIZE);
- assertEquals(factory.getDupsOKBatchSize(),
ActiveMQClient.DEFAULT_ACK_BATCH_SIZE);
- assertEquals(factory.getMinLargeMessageSize(),
ActiveMQClient.DEFAULT_MIN_LARGE_MESSAGE_SIZE);
- assertEquals(factory.getProducerMaxRate(),
ActiveMQClient.DEFAULT_PRODUCER_MAX_RATE);
- assertEquals(factory.getConfirmationWindowSize(),
ActiveMQClient.DEFAULT_CONFIRMATION_WINDOW_SIZE);
+ assertEquals(ActiveMQClient.DEFAULT_CALL_TIMEOUT,
factory.getCallTimeout());
+ assertEquals(ActiveMQClient.DEFAULT_CLIENT_FAILURE_CHECK_PERIOD,
factory.getClientFailureCheckPeriod());
+ assertEquals(null, factory.getClientID());
Review Comment:
Should use assertNull
--
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
