Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2116#discussion_r191793991
  
    --- Diff: 
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/SessionMetadataAddExceptionTest.java
 ---
    @@ -75,6 +93,37 @@ public void testInvalidClientIdSetFactory() throws 
Exception {
           cf.createConnection();
        }
     
    +   @Test(timeout = 5000, expected = JMSException.class)
    +   public void testDuplicateClientIdSet() throws Exception {
    +      ActiveMQConnectionFactory activeMQConnectionFactory = 
(ActiveMQConnectionFactory) cf;
    +      Connection con = cf.createConnection();
    +      Connection con2 = cf.createConnection();
    +      try {
    +         con.setClientID("valid");
    +         con2.setClientID("valid");
    +         fail("Should have failed for duplicate clientId");
    +      } catch (InvalidClientIDException e) {
    +         assertEquals(1, duplicateCount.get());
    +         throw e;
    --- End diff --
    
    Problem here is assert will be bypassed and test will still pass if another 
exception thrown as tests expects JMSException.
    
    Expects should be explicit and need a catch all with and assert fail


---

Reply via email to