[GitHub] [accumulo] ctubbsii commented on pull request #3712: Refactor assertTrue(!=) with assertNotEquals & Add explanatory messages

["ctubbsii (via GitHub)" <gi...@apache.org>]

ctubbsii commented on PR #3712:
URL: https://github.com/apache/accumulo/pull/3712#issuecomment-1687050243

   > Thanks, @EdColeman and @ctubbsii, for your feedback.
   > 
   > I have removed the assertion messages as suggested. I wanted to raise the point on having multiple assert statements in a single test method, which is known in the research literature as a test smell called _ Assertion Roulette_.
   
   I'm not familiar with that literature, but after a quick Google search, I understand it to be not about multiple assertions themselves, but multiple assertions without sufficient description to identify which of the assertions failed for a given test. However, the general concerns about that are already addressed by using a test framework like JUnit, where the assertion failures already identify the specific line in the code where the assertion failed.
   
   For example, the following would be an example of Assertion Roulette:
   
   ```java
     public void testMethodAssertionRouletteBadSmall() {
       if (condition1) {
         throw new AssertionError();
       }
       if (condition1) {
         throw new AssertionError();
       }
       if (condition2) {
         throw new AssertionError();
       }
       if (condition3) {
         throw new AssertionError();
       }
     }
   ```
   
   But, the following would not be, because JUnit adds sufficient information about which specific assertion failed and for what reason:
   
   ```java
     public void testMethodIsOkay() {
       assertThat(condition1);
       assertThat(condition2);
       assertThat(condition3);
     }
   ```
   
   We can add more details in JUnit's API, but often, we don't need to, because it's already implied by the choice of which assertion method to use from JUnit.
   
   So, I don't think our code is a good example of Assertion Roulette.
   
   There are some places in our code that might be, though, where we do something like:
   
   ```java
     public void testMethodAssertionRouletteBadSmall() {
       manyConditions.forEach(condition -> assertThat(condition));
     }
   ```
   
   These are probably better off with something more like:
   
   ```java
     public void testMethodAssertionRouletteIsBetter() {
       manyConditions.forEach(condition -> assertThat(condition, condition.description()));
     }
   ```
   
   But even then, we don't do that kind of thing very often, and if something does fail in a loop like that, it's still plenty valuable enough to track down the problem, without needing to bloat the test code with more complexity to avoid the bad smell.


-- 
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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org