Simplify unit test display names

[FanJups]

Closes #82

There is already a class DisplayNameGenerator.ReplaceUnderscores.class handling underscores but this case is more complex than just replacing underscore with space.

• Implement core logic

• Make String replaceCamelCaseAndUnderscoreAndNumber(String input) private

• Use static final DisplayNameGenerator INSTANCE = new ReplaceCamelCaseAndUnderscoreAndNumber()

• Add parameterized tests to the class annotated with the custom generator

• Use TestInfo to test the display names inside ClassAnnotatedWithReplaceCamelCaseAndUnderscoreAndNumberTest without explicitly calling the method generateDisplayNameForMethod .

• Make it available to be used globally by any modules

  • Solve the module issue related to the usage of @DisplayNameGeneration(ReplaceCamelCaseAndUnderscoreAndNumber.class) - TestEngine with ID 'junit-jupiter' failed to discover tests

• Update TESTING-GUIDELINE

[KyleAure]

Good first pass! You currently have a build error due to lack of copyright headers.

[FanJups]

Ok thanks @KyleAure for your reviews. I will apply them.

[eliasnogueira]

Thank you for this PR!
I have an uncomfortable, but honest question: what's the main benefit of this PR?

The @DisplayName method sometimes will have the same method name, and its intent is to provide better naming for reports and explanatory purposes.

Adding code to transform the method name into the display name is useless ass the method name will be there without the @DisplayName annotation.

[FanJups]

Thank you for this PR! I have an uncomfortable, but honest question: what's the main benefit of this PR?

The @DisplayName method sometimes will have the same method name, and its intent is to provide better naming for reports and explanatory purposes.

Adding code to transform the method name into the display name is useless ass the method name will be there without the @DisplayName annotation.

Thanks @eliasnogueira for sharing your thought. As you said, sometimes there is a pattern, instead of duplicating @DisplayName, we will just use @DisplayNameGeneration at class level. Then the developer will only use @DisplayName in case of a naming not matching the pattern or wanting to customize display name.

About method name transformation, sure we could use the method name itself but readability may decrease.

Let's illustrate it better, shouldReturn5Errors is really easy to read but what about this one shouldReturnTheNumberOfErrorsAs_numberOfErrors_InferiorOrEqualTo5 ? Is it not better Should return the number of errors as numberOfErrors inferior or equal to 5 ?

[FanJups]

To conclude, we are just doing the same thing as inner classes of DisplayNameGenerator do but with custom rules.

[eliasnogueira]

Thank you for this PR! I have an uncomfortable, but honest question: what's the main benefit of this PR?
The @DisplayName method sometimes will have the same method name, and its intent is to provide better naming for reports and explanatory purposes.
Adding code to transform the method name into the display name is useless ass the method name will be there without the @DisplayName annotation.

Thanks @eliasnogueira for sharing your thought. As you said, sometimes there is a pattern, instead of duplicating @DisplayName, we will just use @DisplayNameGeneration at class level. Then the developer will only use @DisplayName in case of a naming not matching the pattern or wanting to customize display name.

About method name transformation, sure we could use the method name itself but readability may decrease.

Let's illustrate it better, shouldReturn5Errors is really easy to read but what about this one shouldReturnTheNumberOfErrorsAs_numberOfErrors_InferiorOrEqualTo5 ? Is it not better Should return the number of errors as numberOfErrors inferior or equal to 5 ?

I got your point.
I personally like to write the test name output in the @DisplayName rather than write it in the method name itself.
Using your example, both are the same and readable:

• shouldReturnTheNumberOfErrorsAs_numberOfErrors_InferiorOrEqualTo5
• Should return the number of errors as numberOfErrors inferior or equal to 5

Thank you for this PR! I have an uncomfortable, but honest question: what's the main benefit of this PR?
The @DisplayName method sometimes will have the same method name, and its intent is to provide better naming for reports and explanatory purposes.
Adding code to transform the method name into the display name is useless ass the method name will be there without the @DisplayName annotation.

Thanks @eliasnogueira for sharing your thought. As you said, sometimes there is a pattern, instead of duplicating @DisplayName, we will just use @DisplayNameGeneration at class level. Then the developer will only use @DisplayName in case of a naming not matching the pattern or wanting to customize display name.

About method name transformation, sure we could use the method name itself but readability may decrease.

Let's illustrate it better, shouldReturn5Errors is really easy to read but what about this one shouldReturnTheNumberOfErrorsAs_numberOfErrors_InferiorOrEqualTo5 ? Is it not better Should return the number of errors as numberOfErrors inferior or equal to 5 ?

To be honest, this is kind of overengineeing. I appreciate your time and effort added here, trying to contribute. On the other hand, you added something that's already a feature from JUnit 5.

We can achieve the same thing you implemented using the Setting the Default Display Name Generator from the official JUnit 5 documentation, where we can simply set a property to have the same result you developed.

In your opinion, would you go toward a custom implementation, that already exists as a supported feature from the library, or would you use the library feature?

Going with the first approach will generate more work and maintainability from the project side compared to using the same feature that the framework already provides.

I appreciate your time and effort, but I cannot see any tangible benefit in this PR.
@otaviojava @KyleAure any thoughts?

[KyleAure]

@eliasnogueira Thanks for your feedback. I was hoping for this type of feedback when I opened issue #82. I just thought it was weird that we were using @DisplayName to convey duplicate information that was already described within the test name itself. So having some sort of way to reduce the redundancy was my goal in opening the issue.

Since I never got any feedback I figured I was the only one who was bothered by it :)

We have 3 options:

1. keep things the way they are now, some what redundant but not over engineered.
2. switch to using ReplaceUnderscores which is built-in to JUnit but makes test method names a bit ugly.
3. use the custom DisplayNameGenerator from @FanJups

I'm fine with any of these options.

[eliasnogueira]

@KyleAure somehow I missed the issue :(

I would go with item 2, adding the property to use the ReplaeUnderscors without adding any additional code, so we can provide both ways: explicitly adding the @DisplayName which will have the priority even with the feature enabled and letting JUnit generate it in the case we don't add.

[FanJups]

I think we could keep this custom generator then the dev will use it or not. At least, the dev will have another option

[FanJups]

@KyleAure , please are you availabl these days for a call to help me figuring out how to solve setup issues. I sent you an email last week.

[FanJups]

The custom generator is not ready to be used because it should be registered first by updating JUnit config.
This error occurs when it is used: TestEngine with ID 'junit-jupiter' failed to discover tests
I Will figure out how to solve this new issue.

[FanJups]

To be honest, this is kind of overengineeing. I appreciate your time and effort added here, trying to contribute. On the other hand, you added something that's already a feature from JUnit 5.

We can achieve the same thing you implemented using the Setting the Default Display Name Generator from the official JUnit 5 documentation, where we can simply set a property to have the same result you developed.

In your opinion, would you go toward a custom implementation, that already exists as a supported feature from the library, or would you use the library feature?

@eliasnogueira sorry for replying late, the main goal is not to make this custom generator the default one, I am thinking about a way to let the developer choose to use it or not.

Yesterday, I noticed that my tests were always failing because I was using that annotation. I got this message so many times:
TestEngine with ID 'junit-jupiter' failed to discover tests

Commenting that annotation made tests pass.

After searching, I also got that JUnit link you shared helping me to understand that maybe it's because I have not yet registered this new custom generator. That's a good point but I don't want to make it default. I just want JUnit to know there is a new generator available and not complain that it fails to discover tests.

I agree with you that there is no need to use this annotation @DisplayNameGeneration after setting it as default generator through properties.

About readability, it's a little bit subjective because I wear glasses, I feel like the longer the method name, the more complicated it is to read as display name. We have our own preferences, this is another reason to not make it default.

To sum up, I am thinking about a way to register this new generator without making it default as if this generator is a JUnit built-in generator (like Standard, Simple, ReplaceUnderscores and IndicativeSentences) then not required to be set as default to be used.

[FanJups]

After uncommenting annotation and paying attention to the logs, I am on the right path to avoid making it as default but have to solve this module issue

Internal Error occurred.
org.junit.platform.commons.JUnitException: TestEngine with ID 'junit-jupiter' failed to discover tests
	at org.junit.platform.launcher.core.EngineDiscoveryOrchestrator.discoverEngineRoot(EngineDiscoveryOrchestrator.java:160)
	at org.junit.platform.launcher.core.EngineDiscoveryOrchestrator.discoverSafely(EngineDiscoveryOrchestrator.java:132)
	at org.junit.platform.launcher.core.EngineDiscoveryOrchestrator.discover(EngineDiscoveryOrchestrator.java:107)
	at org.junit.platform.launcher.core.EngineDiscoveryOrchestrator.discover(EngineDiscoveryOrchestrator.java:78)
	at org.junit.platform.launcher.core.DefaultLauncher.discover(DefaultLauncher.java:99)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:85)
	at org.junit.platform.launcher.core.DelegatingLauncher.execute(DelegatingLauncher.java:47)
	at org.junit.platform.launcher.core.SessionPerRequestLauncher.execute(SessionPerRequestLauncher.java:63)
	at com.intellij.junit5.JUnit5IdeaTestRunner.startRunnerWithArgs(JUnit5IdeaTestRunner.java:57)
	at com.intellij.rt.junit.IdeaTestRunner$Repeater$1.execute(IdeaTestRunner.java:38)
	at com.intellij.rt.execution.junit.TestsRepeater.repeat(TestsRepeater.java:11)
	at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:35)
	at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:235)
	at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:54)
Caused by: org.junit.platform.commons.JUnitException: MethodSelector [className = 'jakarta.data.displaynamegeneration.test.ReplaceCamelCaseAndUnderscoreAndNumberTest', methodName = 'shouldReturnMethodDisplayNamesForCamelCaseAndUnderscoreAndNumber', parameterTypes = 'java.lang.String,java.lang.String', classLoader = null] resolution failed
	at org.junit.platform.launcher.listeners.discovery.AbortOnFailureLauncherDiscoveryListener.selectorProcessed(AbortOnFailureLauncherDiscoveryListener.java:39)
	at org.junit.platform.engine.support.discovery.EngineDiscoveryRequestResolution.resolveCompletely(EngineDiscoveryRequestResolution.java:103)
	at org.junit.platform.engine.support.discovery.EngineDiscoveryRequestResolution.run(EngineDiscoveryRequestResolution.java:83)
	at org.junit.platform.engine.support.discovery.EngineDiscoveryRequestResolver.resolve(EngineDiscoveryRequestResolver.java:113)
	at org.junit.jupiter.engine.discovery.DiscoverySelectorResolver.resolveSelectors(DiscoverySelectorResolver.java:46)
	at org.junit.jupiter.engine.JupiterTestEngine.discover(JupiterTestEngine.java:69)
	at org.junit.platform.launcher.core.EngineDiscoveryOrchestrator.discoverEngineRoot(EngineDiscoveryOrchestrator.java:152)
	... 13 more
Caused by: java.lang.reflect.InaccessibleObjectException: Unable to make public jakarta.data.displaynamegeneration.ReplaceCamelCaseAndUnderscoreAndNumber() accessible: module jakarta.data does not "exports jakarta.data.displaynamegeneration" to unnamed module @7b69c6ba
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:354)
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
	at java.base/java.lang.reflect.Constructor.checkCanSetAccessible(Constructor.java:188)
	at java.base/java.lang.reflect.Constructor.setAccessible(Constructor.java:181)
	at org.junit.platform.commons.util.ReflectionUtils.makeAccessible(ReflectionUtils.java:1770)
	at org.junit.platform.commons.util.ReflectionUtils.newInstance(ReflectionUtils.java:553)
	at org.junit.platform.commons.util.ReflectionUtils.newInstance(ReflectionUtils.java:528)
	at org.junit.jupiter.engine.descriptor.DisplayNameUtils.lambda$findDisplayNameGenerator$6(DisplayNameUtils.java:140)
	at java.base/java.util.Optional.map(Optional.java:260)
	at org.junit.jupiter.engine.descriptor.DisplayNameUtils.findDisplayNameGenerator(DisplayNameUtils.java:127)
	at org.junit.jupiter.engine.descriptor.DisplayNameUtils.lambda$createDisplayNameSupplier$5(DisplayNameUtils.java:117)
	at org.junit.jupiter.engine.descriptor.DisplayNameUtils.determineDisplayName(DisplayNameUtils.java:89)
	at org.junit.jupiter.engine.descriptor.JupiterTestDescriptor.<init>(JupiterTestDescriptor.java:69)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.<init>(ClassBasedTestDescriptor.java:101)
	at org.junit.jupiter.engine.descriptor.ClassTestDescriptor.<init>(ClassTestDescriptor.java:51)
	at org.junit.jupiter.engine.discovery.ClassSelectorResolver.newClassTestDescriptor(ClassSelectorResolver.java:119)
	at org.junit.jupiter.engine.discovery.ClassSelectorResolver.lambda$resolve$0(ClassSelectorResolver.java:71)
	at org.junit.platform.engine.support.discovery.EngineDiscoveryRequestResolution$DefaultContext.createAndAdd(EngineDiscoveryRequestResolution.java:250)
	at org.junit.platform.engine.support.discovery.EngineDiscoveryRequestResolution$DefaultContext.addToParent(EngineDiscoveryRequestResolution.java:213)
	at org.junit.jupiter.engine.discovery.ClassSelectorResolver.resolve(ClassSelectorResolver.java:71)
	at org.junit.platform.engine.support.discovery.EngineDiscoveryRequestResolution.lambda$resolve$2(EngineDiscoveryRequestResolution.java:135)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
	at java.base/java.util.ArrayList$ArrayListSpliterator.tryAdvance(ArrayList.java:1602)
	at java.base/java.util.stream.ReferencePipeline.forEachWithCancel(ReferencePipeline.java:129)
	at java.base/java.util.stream.AbstractPipeline.copyIntoWithCancel(AbstractPipeline.java:527)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:513)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
	at java.base/java.util.stream.FindOps$FindOp.evaluateSequential(FindOps.java:150)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.findFirst(ReferencePipeline.java:647)
	at org.junit.platform.engine.support.discovery.EngineDiscoveryRequestResolution.resolve(EngineDiscoveryRequestResolution.java:189)
	at org.junit.platform.engine.support.discovery.EngineDiscoveryRequestResolution.resolve(EngineDiscoveryRequestResolution.java:126)
	at org.junit.platform.engine.support.discovery.EngineDiscoveryRequestResolution.access$100(EngineDiscoveryRequestResolution.java:58)
	at org.junit.platform.engine.support.discovery.EngineDiscoveryRequestResolution$DefaultContext.resolve(EngineDiscoveryRequestResolution.java:228)
	at org.junit.platform.engine.support.discovery.EngineDiscoveryRequestResolution$DefaultContext.addToParent(EngineDiscoveryRequestResolution.java:222)
	at org.junit.jupiter.engine.discovery.MethodSelectorResolver$MethodType.resolve(MethodSelectorResolver.java:208)
	at org.junit.jupiter.engine.discovery.MethodSelectorResolver$MethodType.access$300(MethodSelectorResolver.java:164)
	at org.junit.jupiter.engine.discovery.MethodSelectorResolver.lambda$resolve$0(MethodSelectorResolver.java:93)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
	at java.base/java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:992)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
	at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:682)
	at org.junit.jupiter.engine.discovery.MethodSelectorResolver.resolve(MethodSelectorResolver.java:97)
	at org.junit.jupiter.engine.discovery.MethodSelectorResolver.resolve(MethodSelectorResolver.java:75)
	at org.junit.platform.engine.support.discovery.EngineDiscoveryRequestResolution.lambda$resolve$2(EngineDiscoveryRequestResolution.java:150)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
	at java.base/java.util.ArrayList$ArrayListSpliterator.tryAdvance(ArrayList.java:1602)
	at java.base/java.util.stream.ReferencePipeline.forEachWithCancel(ReferencePipeline.java:129)
	at java.base/java.util.stream.AbstractPipeline.copyIntoWithCancel(AbstractPipeline.java:527)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:513)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
	at java.base/java.util.stream.FindOps$FindOp.evaluateSequential(FindOps.java:150)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.findFirst(ReferencePipeline.java:647)
	at org.junit.platform.engine.support.discovery.EngineDiscoveryRequestResolution.resolve(EngineDiscoveryRequestResolution.java:189)
	at org.junit.platform.engine.support.discovery.EngineDiscoveryRequestResolution.resolve(EngineDiscoveryRequestResolution.java:126)
	at org.junit.platform.engine.support.discovery.EngineDiscoveryRequestResolution.resolveCompletely(EngineDiscoveryRequestResolution.java:92)
	... 18 more

Process finished with exit code -2

[KyleAure]

Do these methods need to be annotated with @Test? I don't think that the ReplaceCamelCaseAndUnderscoreAndNumber actually checks that the method passed in is annotated with @Test

Having these annotated will just clog the test results.

[FanJups]

Sure, these methods must be annotated with @ Test or @ParameterizedTest. I don't think that we need to check if this method is annotated correctly because by default this generator will only focus on test methods.
As you can see, the parent class DisplayNameGenerator.Standard does not check anything

		@Override
		public String generateDisplayNameForMethod(Class<?> testClass, Method testMethod) {
			String displayName = testMethod.getName();
			if (hasParameters(testMethod)) {
				displayName += ' ' + parameterTypesAsString(testMethod);
			}
			return displayName;
		}

Then, ReplaceCamelCaseAndUnderscoreAndNumberTest must check only the method annotated correctly. I will update

[FanJups]

About readability, it's a little bit subjective because I wear glasses, I feel like the longer the method name, the more complicated it is to read as display name. We have our own preferences, this is another reason to not make it default.

@eliasnogueira , I just want to come back on readability by sharing Igorski's article.

Test names usually tend to become very long and difficult to read. Depending on the naming convention you use, you could have some mandatory words like "should" or "test" or you could be using snake case. All of that contributes to longer names which makes the test results difficult to read in test reports, in IDEs and build tools.

[FanJups]

Hi @KyleAure , please I need your help to Solve the module issue related to the usage of @DisplayNameGeneration(ReplaceCamelCaseAndUnderscoreAndNumber.class) - TestEngine with ID 'junit-jupiter' failed to discover tests

[KyleAure]

I was able to resolve the TestEngine with ID 'junit-jupiter' failed to discover tests by:

1. Moving the test classes to the same directory as the generator class
2. Removing @Disabled

Even with that the reporter did not use the generator. The surefire plugin needs to be configured to use the new generated names:

    <build>
        <plugins>
            <plugin>
	          <groupId>org.apache.maven.plugins</groupId>
	          <artifactId>maven-surefire-plugin</artifactId>
	          <version>3.2.1</version>
	          <configuration>
		      <statelessTestsetReporter implementation="org.apache.maven.plugin.surefire.extensions.junit5.JUnit5Xml30StatelessReporter">
		          <usePhrasedTestCaseMethodName>true</usePhrasedTestCaseMethodName>
		          <usePhrasedTestCaseClassName>true</usePhrasedTestCaseClassName>
		      </statelessTestsetReporter>
	          </configuration>
            </plugin>
        </plugins>	
	</build>

It also seems like @ParameterizedTest tests are excluded from DisplayNameGeneration.
Since this @DisplayNameGeneration doesn't seem to be a viable, nor well supported way to accomplish this task I do not think it would be worthwhile exploring this any further. I think updates to JUnit 5 would be more appropriate.

[FanJups]

Ok thanks @KyleAure for your review. I will propose this custom generator to JUnit team. I had the same idea weeks ago but I wanted to do it here instead of JUnit side. Finally, looks like the best solution.

[FanJups]

This feature will be implemented by JUnit Pioneer junit-pioneer/junit-pioneer#819