consistent naming for tests

[John Hendrikx]

On 09/07/2024 11:33, Johan Vos wrote:
> Hi,
>
> An interesting question from John Hendrikx 
> (https://github.com/openjdk/jfx/pull/1283/#discussion_r1637684395) 
> probably needs some general discussion on this list.
> Afaik, we don't have guidelines for how to name tests (in 
> https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md#coding-style-and-testing-guidelines)
>
> In the different test files we have, I see different patterns:
> * in some cases, tests are always prefixed with `test` (e.g. `testFoo()`)
> * in some cases, tests have a concise but somehow meaningful name 
> (e.g. `testScrollBarStaysVisible`)
> * in some cases, tests refer to JBS issues (e.g. testJDK8309935)
> * in some cases, the test is explained in comments.
>
> I think it would be good to have some consistency going forward. (I'm 
> not advocating we need to rename existing tests!)
> I don't have a strong preference myself, but I think the link to the 
> JBS issue that triggered the creation of a specific test is always 
> good to have. I am also very ok with comments, but I learned not 
> everyone likes that.

Hi Johan,

I didn't like the test name primarily because it has an indirection in 
it -- to know what it is supposed to do, I need to go to another system, 
log in, and read the issue.  The issue often however also won't really 
have just one problem or just a tiny description.  So although I think 
it is fine to refer to an issue number, I think the test itself would 
still benefit of a reminder what it is testing (and I see you added a 
comment now, so that helps).

Same goes for comments in the code; I often just see code lines marked 
with "RT-xyz" or "JDK-xyz" -- sometimes referring a non-public issue.  
Just a small description in the code what the problem was is way more 
helpful to (future) maintainers, like ourselves, then a random number 
without further explanation.

As for test naming, it will depend a bit on what you're testing. 
Sometimes using a nested class pattern like this is good:

     class ListTest {
         @Nested
         class WhenEmpty {
               void thenIsEmptyShouldReturnTrue() {
                    assertThat(list.isEmpty()).isTrue();
               }
               void thenSizeShouldBe0() {
                    assertThat(list.size()).isEqualTo(0);
               }
               void thenGettingFirstElementShouldThrowException() {
                    assertThatThrownBy(() -> list.get(0))
.isInstanceOf(IndexOutOfBoundsException.class)
                         .hasMessage("index 0");
               }

               @Nested
               class AndFiveElementsAreAdded {
                     // add 5 elements in beforeEach

                     void thenIsEmptyShouldReturnFalse() {}
                     // etc
               }
         }

         // or if nesting becomes too crazy, start from base again:
         @Nested
         class WhenContainingFiveElements {
               void thenIterationShouldReturnThoseFiveElements() {
                     assertThat(list).containsExactly(List.of("A", "B", 
"C", "D", "E"));
               }
               // etc.
         }
     }

The above pattern has top level nested classes start with "When" 
followed by a given state, nested classes start with "And" and a 
modification of the state, and test method naming follows the pattern 
"then <action> should <result>".  When combined with a 
DisplayNameGenerator that transforms camel case names to spaced names, 
the tests form more or less normal English sentences.

(Note: I used AssertJ here for much more fluently readable asserts)

That said, I wouldn't go for a fixed naming pattern for test methods as 
it can be a bit situational, but I do think they should be descriptive.

--John

>
> Thoughts?
>
> - Johan