Clarify differences between DisplayName & Name throughout JUnit 5

[schauder]

I understand the need for a UniqueId and a DisplayName, which I guess is only unique among the children of single a parent. But what is the Name for?

---

Related To

• Clarify and document names and display names of JUnit 4 and 5 TestDescriptors #145
• Introduce SPI for generating display names #162

[jlink]

We discussed the issue today in a team call. I try to summarize.

Current situation:

• The DisplayName property describes the (hopefully) easy to read description of a test case or a test container. It currently defaults to a test method's simple name (without class); in the test container case it describes the classes fully qualified name. The default display name can be overridden by @DisplayName("my display name").
• The Name property is independent from display name. For a container it's the same as the default display name, but in the case of a test it's a concatenation of class name, method name and method parameters.
• The Source property points to a class or a method object.

The behaviour described above seems inconsistent. I therefore suggest the following changes:

• The contents of Name should be the default value for DisplayName.

• Name should result in the simple class name for containers (also for nested containers) and the method name (without params) for tests.

  The information currently coded in the Name property can be retrieved via TestDescriptio.getSource() and - in case of a test - through an additional getParent().getSource(). The downside is that the Eclipse runner will no longer display the package name of a container; but this is already the case if the display name is being changed using @DisplayName (IntelliJ only shows the 'simple' form in all situations). This will no longer be an issue as soon as Eclipse provides native integration for JUnit 5. Having packages as inbetween containers would also mitigate this problem.

• TestInfo gets two additional properties:

  1. Optional<Class> getTestClass
  2. Optional<Method> getTestMethod

Having a simpler form of default display name will also simplify a computed "prettification" of display names (see #162).

[sbrannen]

@jlink, thanks for the detailed write up!

I haven't investigated the proposal in detail with regard to the code and any possible ramifications, but in general the proposal seems like a good idea.

[jlink]

Rethinking the whole issue, I think that getName() is too strongly focused on class-based containers and method based tests (as used by JUnit4 and JUnit5). It does not really make sense for other types of tests like file-based or lambda-based ones. I therefore suggest to get rid of getName() in TestIdentifier and TestDescriptor. The information lost can be got via getSource() (see also #175).

In ExtensionContext and TestInfo getName() could be replaced by Optional<Class<?>> getTestClass() and Optional<Method> getTestMethod().

[marcphilipp]

New Proposal

I also think we should not have two competing naming concepts. In addition, as @akozlova has expressed in #172, IntelliJ wants to know whether a name was explicitly set by users (using @DisplayName in case of JUnit 5) or implicitly generated by the TestEngine (e.g. method or class name). Since they want to shorten the fully qualified class name, we might as well provide a short name (simple class name) and a long name (fully qualified class name) so they don't have to do it themselves. Still, it might make sense to know whether a name is explicit or implicit.

To limit the number of methods we'll have to add to TestIdentifier and TestDescriptor, I propose that we replace getDisplayName() : String and getName() : String with getName() : TestName where TestName is a simple immutable value object that could look like this:

public final class TestName {

    public static TestName implicit(String name) {
        return implicit(name, name);
    }

    public static TestName implicit(String shortName, String longName) {
        return new TestName(false, shortName, longName);
    }

    public static TestName explicit(String name) {
        return explicit(name, name);
    }

    public static TestName explicit(String shortName, String longName) {
        return new TestName(true, shortName, longName);
    }

    private final boolean explicit;
    private final String shortName;
    private final String longName;

    public TestName(boolean explicit, String shortName, String longName) {
        this.explicit = explicit;
        this.shortName = shortName;
        this.longName = longName;
    }

    public boolean isExplicit() {
        return explicit;
    }

    public String getShortName() {
        return shortName;
    }

    public String getLongName() {
        return longName;
    }

    @Override
    public String toString() {
        return longName;
    }

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;
        TestName testName = (TestName) o;
        return explicit == testName.explicit &&
                Objects.equals(shortName, testName.shortName) &&
                Objects.equals(longName, testName.longName);
    }

    @Override
    public int hashCode() {
        return Objects.hash(explicit, shortName, longName);
    }
}

@junit-team/junit-lambda What do you think?

[nipafx]

This looks like a useful abstraction.

One detail: I would forego the boolean explicit and make it into an enum NameOrigin (or whatever) with according values. This makes the code more expressive and leaves the door open to future changes (although I have to admit that I can't come up with one that illustrates my point).

[sbrannen]

After reading through all of the comments and reflecting on the proposals, I agree with most everything that @jlink proposed.

The display name is exactly that: what IDEs and build tools should display as the name in human readable form.

Consequently, IDEs and build tools should never rely on the display name when determining how to structure the test tree in a UI.

IDEs and build tools must therefore rely on the information provided via the concrete TestSource associated with the TestIdentifier.

Furthermore, this line of reasoning makes the proposal in #172 obsolete: IDEs should not care about the source of the display name or its format. They should simply display it.

Proposal

• Remove getName() from TestIdentifier, TestDescriptor, TestInfo, and ExtensionContext.
• Introduce Optional<Class<?>> getTestClass() and Optional<Method> getTestMethod() in TestInfo and ExtensionContext.
• Remove getTestMethod() in TestExtensionContext.
• Clearly define the semantics for default display names for packages, top-level classes, static nested classes, inner classes (i.e., @Nested classes), methods, and lambdas (i.e., dynamically registered tests).
• Implement the semantics for default display names as defined in the previous step.
• Document the semantics for default display names as defined in the previous step.

Default Display Name Semantics

For each common test element type, the following listing provides a description as well as a concrete example based on the TopLevel test class.

• package: fully qualified package name
  • org.example
• top-level class: fully qualified class name
  • org.example.TopLevel
• static nested class: fully qualified class name
  • org.example.TopLevel$StaticNested
• inner class: fully qualified class name
  • org.example.TopLevel$Inner
• method: method name plus parameter type list in parentheses; use simple names for parameter types
  • method1()
  • method2(TestInfo)
• lambda: required to be supplied by the user when the test is dynamically registered
  • dynamic test

package org.example;

public class TopLevel {

    static class StaticNested {}

    @Nested
    class Inner {}

    @Test
    void method1() {}

    @Test
    void method2(TestInfo testInfo) {}

    @TestFactory
    Stream<DynamicTest> dynamicTests() {
        return Stream.of(new DynamicTest("dynamic test", () -> {}));
    }

}

@junit-team/junit-lambda, what do you think about this alternative proposal?

[marcphilipp]

One thing to keep in mind regarding test source: When you inherit a test method from a super class, the test source will point to the method in the super class. So it cannot be used to compute a "display name" by the IDE.

Having said that, I am fine with this proposal!

One minor thing: Omitting the package name from the display name of a top-level class only makes sense when packages are actually present in the tree of test descriptors. As we don't have them right now, I think we should keep using the fully-qualified class name.

[sbrannen]

Team decision:

• Implement the proposal by @sbrannen with the following modifications:
  • methods: use simple names for parameter types
  • classes: use FQCN for all categories

Note: the aforementioned modifications have already been made to the proposal.

[sbrannen]

in progress

[sbrannen]

This issue is blocked until #58 has been completed and merged into master.

[sbrannen]

This issue is no longer blocked since #58 has been completed.

[sbrannen]

@junit-team/junit-lambda, PR #279 has been rebased on master and updated regarding DynamicTest support. As such, it is now ready to be merged into master.

[marcphilipp]

Do we need to discuss display names of classes in a team call again (cf. comment on #279) or are we confident enough that using simple names is better?

[sbrannen]

I didn't make the switch to simple names for classes in the PR yet, but I think we can be confident enough that it's likely the correct decision for the long term. Besides, getting this change in as early as possible with result in the least element of surprise should we decide to do so later. And... this gives us a lot of time (over half a year) to gain feedback from the community.

[sbrannen]

So, @marcphilipp, does that mean this PR is good to go (along with the switch to simple names for all classes)? 😉

[marcphilipp]

I'll take a more thorough look at the PR tonight and let you know. 😉

[sbrannen]

thanks

[sbrannen]

Closing this issue since #279 was merged into master in 307bcb4.