Triplea: Convention? Use @DislayName on tests

[DanVanAtta]

Instead of javadocing a test, or having no description of how a test works, use @DisplayName to give a textual description of what is being checked in a test.

Eg:

@Test
@DisplayName("verify error handling when disk is full")
void errroHandlingOfIoException() {
   when(...).doThrow(new IOException("no space on filesystem"));
   // test ...
}

[ron-murhammer]

I'm fine with this.

[asvitkine]

Seems reasonable.

[DanVanAtta]

Thanks for the feedback. A second question, what exceptions should we allow? In other words, do we have a good sense of when a test is trivial enough to not need a "@DisplayName"? My feeling is we would want to favor having one than not. I'm particularly thinking about how often test cases are pretty obvious when you write them, then even the simplest test becomes non-obvious later when that context is lost.

Or, do we allow no exception and just mandate that we'll have a trivial 'display name' value if a test is also trivial?

edit: NB, most tests are probably not going to be trivial. Trivial tests tend not to be created, afterall, what are they testing?

[RoiEXLab]

@DanVanAtta I think if the method name does already explain the test well enough testIfInvalidInputCausesException() {}, a @DisplayName("Test if invalid input causes an Exception") would only repeat the obvious, which doesn't add any value IMO.

Off-Topic:
I always find it funny how @DanVanAtta just references random github accounts whenever he references some random annotation without putting it in a code block ^^
Some day an actual user will tell us to shut up because they're constantly being spammed with our conversations 😄

[DanVanAtta]

Some day an actual user will tell us to shut up because they're constantly being spammed with our conversations smile

Some day? I wish I could find the example, someone certainly got very annoyed.

There could be value in just having human readable display names. We could consider using the 'replaceCamelCase' generator described in:
https://www.baeldung.com/junit-custom-display-name-generator

[RoiEXLab]

I wish I could find the example, someone certainly got very annoyed.

Right, completely forgot that happened ^^

[DanVanAtta]

I'd like to wrap this one up:

• I'm wondering about exceptions. If this is an advisory convention, then what's the point? We'll probably get into a fight when a reviewer asks for this to be done and the author will state that the display name is trivial and should not be added. The reviewer would likely counter that perhaps some additional information can be added, and perhaps that will go somewhere or perhaps there will be more back and forth.

The point of the convention is to stop that kind of back and forth.

So far, I've observed that generally there is more info that can be added to a display name, that it's a very high bar for a truly trivial assertion, and even then the assertion can be described with slightly more detail than the method name. Method names tend to be shorter, display names can be more wordy and slightly more descriptive.

Thoughts?

[RoiEXLab]

Maybe we could agree that if any reviewer thinks that the method name isn't sufficient _and_ provides a good display name it should be used.
So in the end if someone thinks it's not trivial, then it probably isn't, if everyone agrees that it's trivial then it might be the case.
Would that work?

[ron-murhammer]

Pretty neutral here. I agree with @RoiEXLab approach.

[DanVanAtta]

I like the brainstorming.

Maybe we could agree that if any reviewer thinks that the method name isn't sufficient and provides a good display name it should be used.

My concerns:

• subjective to the reviewer
• adds burden to the review to come up with display names.

Being devils advocate, let's say a new PR author just hates this convention. They refuse to add any display name and then they argue that any method name is already descriptive. That then creates a tedious process where the reviewer needs to essentially write all of the display names for the author. That seems like a negative process as the author probably will not care and will just copy/paste whatever to satisfy the convention, they would be unlikely to be motivated to fix/update/add the display names on their own and would only do so when asked.

On the other hand, if we say "this is our convention [link]", then we close down the back-and-forth hopefully, which is ultimately the goal.

[RoiEXLab]

I agree that it is somewhat objective, but this applies to the whole review process really.
The reviewer is usually always the one deciding if a change is merged or not, so this really only adds another decision to make which shouldn't really be a big deal.

Regarding having to come up with a good display name:
Goal is to avoid a reviewer being like "I want a name here", but has no idea on what to call it, neither has the creator of the PR.
That's when things get akward, because in this case it really seems like a silly rule.

In contrast if this really were our convention to add it everywhere, then this might encourage PR creators to just copy paste the method name and make it a real sentence, which doesn't add any value. In this case the reviewer would have to make a suggestion anyways (because nothing is worse than non-constructive feedback), so nothing changes there.

[DanVanAtta]

I think this one is probably decided, we'll need to see how it plays out a bit with 'trivial' vs not. My 2 cents, so far using display name I've found:

• method naming varies enough from a display string that a display string is almost always valuable
• the readability of a display string, even if just un-camel-casing is arguably valid.

I suspect we should see 90%+ unit test methods having a display name on them.

Any final thoughts?

[DanVanAtta]

Added to wiki: https://github.com/triplea-game/triplea/wiki/Java-Test-Code-Conventions#favor-adding-displayname-to-each-test-case-5525