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
