Add @JavaTestMethod and @JavaTestClass annotations to Description when they are known by built-in runners

[dsaff]

Description currently has methods getTestClass, getClassName, and getMethodName. These are lame implementations that depend on parsing information out of the description string that either (1) were already known at the time the Description was created, or (2) aren't there, because the Description doesn't refer to a Java code element at all.

There have been proposals (for example, in https://github.com/KentBeck/junit/pull/195) to further enrich this API with getTestMethod, etc. Instead, let's deprecate this way of doing things, and instead augment the implementation of the built-in Runners and classes that touch Java classes, so that they use the existing Annotation-based metadata mechanism on Description. Thus, for example, to get the Java class associated with a Description, one would use

Class<?> testClass = myDescription.getAnnotation(JavaTestClass.class).getValue();

We could then either deprecate the old Java-reflection-specific methods on Description, or port them over to use the new, clean Annotation implementation.

[aisrael]

Sorry, but I'm unclear as to how JUnit will supply values for the @JavaTestClass or @JavaTestMethod on a class at run-time.

[dsaff]

Descriptions are created with a list of Annotations. You can create anonymous implementations of Annotation interfaces at runtime. It's ugly, but most likely not to break what already exists...

[aisrael]

Seems like an awful roundabout way to pass around target information. I dunno—apologies, and maybe it's just me, but I really don't see what gain that approach has over (or, conversely, what's wrong with) simple getTestClass() or getTestMethod()...

I think that simpler, proven, and less effort (code) is generally preferable over something else—unless, like I said, there's some other benefit that I'm just not seeing.

[dsaff]

Alistair,

Currently, the only way the test class is stored is through the text of the description, which has to be parsed and classloaded, which is ridiculously expensive and error-prone. Storing it in the annotation map is definitely preferable.

There should definitely be a convenience API for getting at the test class and test method stored in a description. But it shouldn't be based on string parsing.

[dsaff]

This should be not too hard for an interested contributor to pick up, and definitely useful.

[codingricky]

I'd be happy to look at it. Would you be able to provide some sample code illustrating its usage?

[dsaff]

@diusricky, There wouldn't really be any new code that it makes possible. It's more about cleaning up what's already there. Description was always supposed to be able to represent any conceivable node in a test tree, not just Java classes and methods, so having public members in the API that make it look like there's always a class and method available is confusing.

I'm imagining a JavaDescription class with only static utility methods for Descriptions, and two annotation classes, @javatestclass and @JavaTestMethod, which are not intended to be actually typed into any test files. Then we:

1. Deprecate constructors and factories on Description that take a Class<?>, in favor of factory methods on JavaDescription that add a dynamically-generated instance of JavaTestClass and add it to the annotations passed to the Description.

2. Deprecate accessors on Description that promise to return Java structure information: getTestClass(), getClassName(), getMethodName(). Instead, encourage users to use equivalent classes on JavaDescription, which read the requested information from Description#getAnnotations.

3. After this, fix any usage of the now-deprecated methods throughout the core JUnit code.

[codingricky]

In terms of the Factory methods on JavaDescription class, are you thinking about something like this?

So would this

    public static Description createSuiteDescription(Class<?> testClass) {
        return new Description(testClass, testClass.getName(), testClass.getAnnotations());
    }

be converted to a factory method like...

public class JavaDescription {

public static Description createDescription(final Class<?> testClass) {
        JavaTestClass javaTestClass = new JavaTestClass() {
            public Class<?> getValue() {
                return testClass.getClass();
            }
        };
        List<Annotation> annotations = new ArrayList<Annotation>();
        Collections.addAll(annotations, testClass.getAnnotations());
        annotations.add(javaTestClass);

        return new Description(testClass, testClass.getName(), annotations);
    }

}

Thanks, just want to make sure I'm on the right track.

[dsaff]

Yep, something like that. Thanks!

[codingricky]

Hi, I had a couple more questions to test my understanding.

1. Once this clean up is done, is the fTestClass field in Description redundant? As if we were storing the value in the @javatestclass annotation we shouldn't require this field anymore.
2. Regarding the use of the @JavaTestMethod annotation. Similar to point 1 you had up in your comment, should the process be to deprecate constructors and factory methods on Description that take method names in favour of factory methods on JavaDescription that dynamically add the @JavaTestMethod annotation?

Thanks!

[dsaff]

1. Yes, indeed! That's one of the benefits here.
2. Yes, mostly. We'll still need constructors that take a "test name", which is more general than a Java method name.

[bechte]

I am currently investigating some thoughts on this and I found a restriction which seems to prevent us from doing what was suggested here:

In http://docs.oracle.com/javase/1.5.0/docs/guide/language/annotations.html it says:
"Return types are restricted to primitives, String, Class, enums, annotations, and arrays of the preceding types."

With this restriction it will be hard to pass in arbitrary objects to enrich the meta data for a test. Anyways, it's no problem to have the JavaTestClass to be passed in, but we cannot pass in the JavaTestMethod (pointing to the "Method" or "FrameworkMethod" instance).

With this information, I am struggling whether or not we are on the right track to solve this issue. What if we add a Map containing additional metadata objects that goes along with the annotations, e.g.:

public class JavaDescriptions {
    public static Description forClass(final Class<?> testClass) {
        return forClass(testClass, testClass.getName());
    }

    public static Description forClass(final Class<?> testClass, String displayName) {
        Map<String, Object> metaData = getAdditionalMetaData(testClass);
        return new Description(displayName, testClass.getAnnotations(), metaData);
    }

    private Map<String, Object> getAdditionalMetaData(final Class<?> testClass) {
        Map<String, Object> metaData = new HashMap<String, Object>();
        metaData.put(TestClass.class.getCanonicalName(), new TestClass(testClass));
        metaData.put(FrameworkMethod.class.getCanonicalName(), /* ... */);

        /* ... */

        return metaData;
    }
}

Hopefully, I didn't miss a point here… If not, I would like to take one step back and revisit the Description class. We might want to redesign the class so that no reflection is used, i.e. only the information given at construction time of the Description class will be returned. Still, this leads to an implementation where we are required to change the constructor for every new piece of information on the Description class. But what if we combine the meta-data approach from above, passing the Map into the Description class and provide getter for the most general information and a accessor for an arbitrary map key to retrieve all other information?

[kcooney]

@bechte I'm not sure we want the attach any kind of object to a Description. For instance, it seems strange to me to pass in the TestClass (before doing that, I might reconsider making TestClass keep a static cache of class to `TestClass instance, but that's a bit of a digression).

That being said, I don't quite understand the motivation for using annotations to access data to Description vs. plain old simple methods. @dsaff can you talk about the motivations a bit more?

[kcooney]

BTW, if we wanted to allow arbitrary data to be added, we could simply use a marker interface:

public class JavaDescription {
  private final Map<Class<T extends Value>, Value> metaData;

  public interface Value {};

  public <T extends Value> T getValue(Class<T> type) {
    return type.cast(metaData.get(type));
  }
}

Edit: My concern with arbitrary data is that I really want the new builders to build immutable Description objects, and I don't want to create a back door to add mutable data.

And, in fact, we want to make sure that Description objects honor the contract for equals() and hashCode(). Mutable objects might not have consistent hashCode() values, so that's another dink against arbitrary data in Description instances.