[FEATURE REQUEST] Rename @Case to something better

[toolforger]

[FEATURE REQUEST] Rename @Case to something better

[CC007]

I don't think that the proposed Element annotation would ever conflict with that API, except when someone tests an annotation processor that processes annotations that are used in an annotation processor (like google testing their @AutoService annotation processor or something), but if you indeed remove the case from the annotation, you can rename it to @Label.

I do think that it would be beneficial if there still was a @CaseLabel annotation or something and that you could use a @ParameterizedTest with a @CaseLabelSource to which you provide the label that was used in the @CaseLabel annotation.

[CC007]

For the single() implementation you can also use elements.values().get(0). That way you can remove the unnecessary loop and throw new IllegalStateException("This exception should never be thrown");

[Pante]

For the single() implementation you can also use elements.values().get(0). That way you can remove the unnecessary loop and throw new IllegalStateException("This exception should never be thrown");

element.values() returns a Collection which doesn't have a get(index) method. Although now that you mention it, I could convert it into an array and retrieve the single element.

Also, you could use the TestAbortedException (which is also used by Junit Jupiter uses for test assumptions), instead of the IllegalStateExceptions

I did think about it, but I figured an IllegalStateException works just as well without having to include an additional dependency.

[Pante]

Hearing no other objections, I'll be going ahead with the proposed changes and finalizing them.

[Pante]

I do think @Case is an appropriate name though since I do think it's similar in concept to that of case distinction. In my mind, we might want to test a single component/lint against various different configurations/cases. In a sense, there are sort of test cases.

For example, we might want to test the behavior of a lint in different cases to ensure that there are no false positive/negatives:

@Case("valid")
void foo(String param) {}

@Case("missing parameter")
void foo() {}

@Case("has return type")
String void(String param)

At least, this is how I envisioned the @Case annotation to be used.

On another note, @Element won't be a good choice since it'll conflict with Java's own Element type.

[toolforger]

Took me a bit to re-read the article and Case.java, but I have a somewhat clearer picture now indeed, and yes it's clear that @Case was intended to be something like a switch label like in a Java switch statement.

It can be used for other purposes as well.

E.g. I can imagine situations where I want to write a test that accesses some attribute, e.g. to pick up its name to double-check that generated getter/setter/wither names are consistent with the original name despite attached prefixes and suffixes.

Or e.g. the annotation processor will accept overrides and not generate code provided, say, there's a setter with the right name and parameter type:

@Bean
private class BeanTemplate // Not necessarily code that's useful at runtime, just a spec for the annotation processor
  @Case("attribute") private int attr;
  private boolean optional;
  @Case("correct_setter_parameters") public void setAttr(int attr) {
    this.attr = attr;
  }
  @Case("wrong parameter type on setter") public void setAttr(int attr) {
    this.attr = attr;
  }
  @Case("too many parameters on setter") public void setAttr(int attr, boolean optional) {
    this.attr = attr;
    this.optional = optional;
  }
  @Case("no parameter on setter") public void setAttr() {
    this.attr = "NO VALUE";
  }
}

@Case("correct_setter_parameters"), @Case("wrong parameter type on setter"), @Case("too many parameters on setter"), and @Case("no parameter on setter") are indeed case labels, but @Case("attribute") is just a tag so the test can conveniently pick up the attribute name and type from the test in case somebody refactors the example code and renames the attribute.

[Pante]

If I understand the example correctly, there's two distinct use-cases for @Case,

• something similar to a case in Java's switch statements
• labeling additional properties that can be accessed in tests

I think an interesting question is whether we should generalize both use-cases under a new single annotation or split them into two distinct annotations.

If we're going with generalizing the annotation, perhaps @Label, @CodeSnippet or @Snippet?

[toolforger]

I think yes, that's the use cases.
Though I'd say that the case-analog @Cases are just a specialization of the "we label code snippets" idea: A case label is, at least in C, an ordinary label, you can even gotoit... which is horribly unclean, of course, but it's the concept of a label.

I can see reasons for both @Label and @Snippet:
@Label since the annotation labels an Element.
@Snippet because it's labelling an Element, which is a code snippet. (hmm... @Snippet is a bit less accurate because people generally consider multiple Elements a snippet - e.g. two statements can be a snippet but you can't label them with one annotation, you have to put them into a pair of braces.)

It's still somewhat subjective; I have a slight preference for @Label.
The fully spelled-out name would be @ElementLabel since the annotation labels an Element, but that's probably too long and unwieldy.

Name conflict analysis:

• java.awt.Label. Unlikely to ever be used in modern code.
• java.jfr.Label. Java Flight Recorder. Not sure if anybody is ever going to write an annotation processor test for that.
• Various jdk.internal. and sun. packages. That's internal, do we need to consider that?

[Pante]

Hmmm, seems like @Label is a better candidate then, since looking at it, people may mistakenly assume that @Snippet spans multiple lines/elements.

I don't think most of the name conflicts matter since they belong to quite the niche areas. I daresay @Label will work fine in roughly 90% of all scenarios.

I'll probably rename @Case to @Label on master in that case. It'll probably take awhile to make it to stable though since there's quite significant rework being done to the satisfactory project.

[toolforger]

Regarding satisfactory - judging by the class names, it seems to be very similar to what Hamcrest does.
If that wild guess happens to be correct, it might be a good idea to leave the composability to Hamcrest and merely provide basic predicates like "this is a type".
(Just an idea. I know ideas are cheap :-) )

[Pante]

It's similar that they are matchers but I think that's where the similarity with Hamcrest ends. The idea is to be able to compose matchers for various elements in a declarative manner (as opposed to how most processors imperatively check types).

Those matchers then can automatically produce error messages using Utilitary's message formatting.

The message formatting & matching parts are done, I just need to figure out how to create nice error messages based on the matchers. This part's taking quite awhile to sort out.

[toolforger]

Ah ok then.
Given that not everybody will want to learn how to integrate it, or how to write messages that interact nicely, it would be good if elementary did not depend on satisfactory, i.e. they should either be independent, or satisfactory should depend on elementary. (I know what such decoupling creates work, sometimes in considerable amounts, so I'll understand if you don't want to do this.)

[Pante]

Hmm I'll probably look into splitting it up once I have some spare time. Unfortunately it probably won't happen until at least November.

[CC007]

I feel like this mixes 2 features:

• The ability to get references to specific types of elements that will be used by the annotation processor
• The ability to easily switch over different elements on a case by case basis

Just renaming the @Case annotation wouldn't fully encompass what you want to achieve. Therefore I propose the following:

@Element annotation with the following annotation values:

• label this is to define the name with which to retrieve the element in your test (providing this is required)
• case in case that you want multiple different test cases, one for each test case, similar to a @ParameterizedTest (has a default value)
• value to make it possible to provide a name like @Element("myAwesomeLabel") instead of having to write @Element(label = "myAwesomeLabel") (so label is basically an alias for value)

Let's give an example of how this would be used. Here is the example from the The Problem With Annotation Processors article:

import com.karuslabs.elementary.junit.Cases;
import com.karuslabs.elementary.junit.Tools;
import com.karuslabs.elementary.junit.ToolsExtension;
import com.karuslabs.elementary.junit.annotations.Inline;
import com.karuslabs.utilitary.type.TypeMirrors;

@ExtendWith(ToolsExtension.class)
@Inline(name = "Samples", source = {
"import com.karuslabs.elementary.junit.annotations.Case;",
"",
"class Samples {",
"  @Case(\"first\") String first;",
"  @Case String second() { return \"\";}",
"}"})
class ToolsExtensionExampleTest {
   
   Lint lint = new Lint(Tools.typeMirrors());
   
   @Test
   void lint_string_variable(Cases cases) {
       var first = cases.one("first");
       assertTrue(lint.lint(first));
   }
   
   @Test
   void lint_method_that_returns_string(Cases cases) {
       var second = cases.get(1);
       assertFalse(lint.lint(second));
   }
   
}

Lets rewrite this with the @Element annotation and add a case:

import com.karuslabs.elementary.junit.Tools;
import com.karuslabs.elementary.junit.ToolsExtension;
import com.karuslabs.elementary.junit.annotations.Inline;
import com.karuslabs.utilitary.type.TypeMirrors;
import javax.lang.model.element.Element;

@ExtendWith(ToolsExtension.class)
@Inline(name = "Samples", source = {
"import com.karuslabs.elementary.junit.annotations.Element;",
"",
"class Samples {",
"  @Element(\"first\") String first;",
"  @Element(label = \"second\", case = \"no param\") String second() { return \"\";}",
"  @Element(label = \"second\", case = \"one param\") String second(String param) { return \"\";}",
"  @Element(label = \"second\", case = \"two params\") String second(String param1, String param2) { return \"\";}",
"}"})
class ToolsExtensionExampleTest {
   
   Lint lint = new Lint(Tools.typeMirrors());
   
   @Test
   void lint_string_variable(Element first) {
       assertTrue(lint.lint(first));
   }
   
   @Test
   void lint_method_that_returns_string(Element second) {
       assertFalse(lint.lint(second));
   }
   
}

This would run 4 tests: lint_string_variable() will run once and lint_method_that_returns_string() will run 3 times (once for each case). For the 3 test cases of lint_method_that_returns_string(), the case string will be used as the name of that test, grouped under the test method name, similar to parameterized tests.

You could still use the Cases test method parameter as well (maybe renamed to Elements or something like that), to be more explicit about what Element you wish to fetch and if you want al test cases to be tested within a single test method call (if that's their personal preference or something). It could also be used to get access to the specific case string from the annotation.

[CC007]

For completeness, here is how the test would look with the Cases/Elements test method parameter:

import com.karuslabs.elementary.junit.Tools;
import com.karuslabs.elementary.junit.ToolsExtension;
import com.karuslabs.elementary.junit.annotations.Inline;
import com.karuslabs.utilitary.type.TypeMirrors;
import javax.lang.model.element.Element;
import java.util.List;
import java.util.Map;

@ExtendWith(ToolsExtension.class)
@Inline(name = "Samples", source = {
"import com.karuslabs.elementary.junit.annotations.Element;",
"",
"class Samples {",
"  @Element(\"first\") String first;",
"  @Element(label = \"second\", case = \"no param\") String second() { return \"\";}",
"  @Element(label = \"second\", case = \"one param\") String second(String param) { return \"\";}",
"  @Element(label = \"second\", case = \"two params\") String second(String param1, String param2) { return \"\";}",
"}"})
class ToolsExtensionExampleTest {
   
   Lint lint = new Lint(Tools.typeMirrors());
   
   @Test
   void lint_string_variable(Elements elements) {
       // maybe it would be better if the one(String label) method was renamed to get(String label) and threw a TestAbortedException (the same one that junit5 assumptions throw) rather than returning null if there isn't exactly one element
       Element first = elements.get("first"); 
       assertTrue(lint.lint(first));
   }
   
   @Test
   void lint_method_that_returns_string(Elements elements) {
       // without case string
       List<Element> secondList = elements.asList("second");
       secondList.foreach(second -> {
           assertFalse(lint.lint(second));
       }
       
       // with case string
       Map<String, Element> secondMap = elements.asMap("second");
       secondList.foreach((case, second) -> {
           assertFalse(lint.lint(second), "For case: " + case");
       }
   }
   
}

[CC007]

I also think that Cases.get(int index) can be removed if needed, as it seems very error-prone to retrieve elements without label, especially if you retrieve them from a possibly nonexistent index.

If you do still want the functionality, you can always use Elements.asList("myLabel").get(1);

[Pante]

I feel like both cases can be generalised as “get elements annotated with a label”.

I’m not too particularly fond of cases for parameterised tests as it isn’t obvious from a glance that a test is parameterised since it’s not annotated with @ParameterizedTest. People not familiar with elementary might be caught off-guard by this. I’m also not too sure if it’s possible to re-execute a test marked with @Test but with different parameters each time.

I feel a simpler way will just be to mark a test with @ParameterizedTest and a list of labels instead. If that doesn’t work at the moment, then I think effort should be in this direction instead. Furthermore, it’s less surprising.

I don’t think renaming Case And Cases to Element and Elements is a good idea since they will conflict with Java’s own TypeMirror API. @Label and Labels might be better.

Re-thinking about it, I agree that Cases.get(index) should be removed since it’s extremely confusing & error-prone.

Huh, TIL that TestAbortedException existed, I agree that the method should definitely throw the exception rather than returning null.

Hmm regarding renaming one() to get(), perhaps it will be better to enforce that all labels are unique & maybe provide some forming of matching on that instead.

[toolforger]

On 12.10.22 00:48, CC007 wrote: I feel like this mixes 2 features: * The ability to get references to specific types of elements that will be used by the annotation processor * The ability to easily switch over different elements on a case by case basis

Sure it's two different use cases. Question is whether they need different kinds of support; if both use cases just need to name some element, one could go for the "toolbox" approach: giving the programmer tools rather than precanned workflows.

***@***.***| annotation with the following annotation values: * |label| this is to define the name with which to retrieve the element in your test (providing this is required) * |case| in case that you want multiple different test cases, one for each test case, similar to a ***@***.***| (has a default value) * |value| to make it possible to provide a name like ***@***.***("myAwesomeLabel")| instead of having to write ***@***.***(label = "myAwesomeLabel")| (so |label| is basically an alias for |value|)

Would one still need "case" to make parameterized test cases? Is "case" needed for nonparameterized test cases, does it even make sense? There's also the downside that each additional detail means that the user has to remember it to successfully use it. (In alignment with "perfection is not when there is nothing to add, but nothing to take away", though that just a perspective to keep in mind, not something to tick off.)

This would run 4 tests: |lint_string_variable()| will run once and |lint_method_that_returns_string()| will run 3 times (once for each case). For the 3 test cases of |lint_method_that_returns_string()|, the |case| string will be used as the name of that test, grouped under the test method name, similar to parameterized tests.

Does ***@***.***| automatically trigger tests? In that case, yes the annotations are different. I'm wondering whether ***@***.***| is overlapping with ***@***.***| though. Because then the developer has to keep in mind that ***@***.***| not just marks an element, it also triggers test repetition. Downsides: * @case overlaps with both @Label and @RepeatedTest. * More details to remember. * You can't make a RepeatedTest that accesses _two_ elements (e.g. for testing annotations that check consistency between otherwise unrelated elements). Upsides: * No need to write the case label twice. Of course, you can't just count upsides and downsides, you need to weigh them, taking value and likelyhood into account. I do agree that accessing elements by index is very error-prone. It might be a good idea to allow a name and an index, so you can select an element according to "the Nth element labelled 'foo'" or similar.

[CC007]

Would one still need "case" to make parameterized test cases?
Is "case" needed for nonparameterized test cases, does it even make sense?

case doesn't make sense for non-parameterized test cases, so that would mean that you probably would want a difference between @Label and @CaseLabel, where the latter is only used in parameterized tests. It would be possible to make the case argument optional, meaning that the cases would just have an index. You would just have to think about what to do when case labels with and without case argument are mixed. Do you want to disallow that or not, and if not, how would you handle that.

I think that focussing on the @Label annotation at first would be more important though and if we want elementary to support parameterized tests later, it can always be added then.

Thinking about it though, we might be missing the point here. What are we trying to achieve? All we want is to have access to a certain Element object that has a certain structure. The way we get it doesn't really matter, so long as it is possible to retrieve with the given workaround. Sometimes it would be enough to just have the element for a field declaration or a method declaration, so having to specify the full class around it would be boilerplate that can be added automatically.

import com.karuslabs.elementary.junit.TestElements;
import com.karuslabs.elementary.junit.TestField;
import com.karuslabs.elementary.junit.TestMethod;
import com.karuslabs.elementary.junit.TestClass;
import com.karuslabs.elementary.junit.Tools;
import com.karuslabs.elementary.junit.ToolsExtension;
import com.karuslabs.utilitary.type.TypeMirrors;
import javax.lang.model.element.Element;

@ExtendWith(ToolsExtension.class)
class ToolsExtensionExampleTest {
   
   Lint lint = new Lint(Tools.typeMirrors());
   
   @Test
   @TestFieldElement(label = "thatOneProblematicField", element = "private final @Foo String myPrivateAnnotatedStringField;", imports = {"my.package.Foo"}),
   void lint_string_variable(TestElements elements) {
       Element field = elements.get("thatOneProblematicField");
       assertTrue(lint.lint(field));
   }
   
   @Test
   @TestMethodElement(label = "mainMethod", element = "public static void main(String[] args){return null;}")
   @TestClassElement(label = "myGenericClass",
                     element = "public class Foo<T extends Bar> implements Baz<T> {}", 
                     imports = {"my.package.Foo", "my.package.Bar", "my.otherpackage.Baz"})
   void lint_main_method_and_class(TestElements elements) {
       Element method = elements.get("mainMethod");
       Element clazz = elements.get("myGenericClass");
       assertTrue(lint.lint(field));
       doSomethingWith(clazz);
   }
   
}

Given the specific annotation, we know exactly how to generate the boilerplate around the element (if the imports are defined correctly), so we don't need the tester to deal with that.

[Pante]

I disagree since doing static code analysis in an annotation processor will almost always rely on the Element/TypeMirror API, which IMO is quite a common use-case.

A @CaseLabelSource does sound promising, perhaps we can go with the original idea of having a case/group field on the @Label annotation.

[CC007]

I disagree since doing static code analysis in an annotation processor will almost always rely on the Element/TypeMirror API, which IMO is quite a common use-case.

I do agree that the Element/TypeMirror api is a common use case. What I'm trying to achieve here is actually to still have the class definition be generated behind the scenes, so that the Element objects are available for your test. This will prevent the user from having to write the boilerplate around the element of interest and skip the parts that aren't directly used in the test (eventhough they will still be available when compiling and creating the Element objects).

[Pante]

I think there's a misunderstanding, I just meant that I specifically disagreed with renaming the annotation to @Element. I'm still on the fence regarding the others.

[CC007]

Ah ok. The annotation processor does indeed heavily rely on Element, but the code that actually would be given the @Element annotation wouldn't usually rely on Element, right? As this would be the compilation target.

[Pante]

I think the main issue is that during the test we want access to both Java's Element and the @Element annotation.

Sometimes it would be enough to just have the element for a field declaration or a method declaration, so having to specify the full class around it would be boilerplate that can be added automatically.

Re-reading this it seems like I missed out this part, I think we already have a far superior mechanism for eliminating boilerplate code, the @Introspect annotation, it allows us to declare @Cases in the test class itself.

@ExtendWith(ToolsExtension.class)
@Introspect
class ToolsExtensionExampleTest {

    Lint lint = new Lint(Tools.typeMirrors());
    
    @Test
    void lint_string_variable(Cases cases) {
        var first = cases.one("first");
        assertTrue(lint.lint(first));
    }

    @Case("first") String foo() { return "";}
    
}

class Lint {
    
    final TypeMirrors types;
    final TypeMirror expectedType;
    
    Lint(TypeMirrors types) {
        this.types = types;
        this.expectedType = types.type(String.class);
    }
    
    public boolean lint(Element element) {
        if (!(element instanceof VariableElement)) {
            return false;
        }
        
        var variable = (VariableElement) element;
        return types.isSameType(expectedType, variable.asType());
    }
    
}

This feature does not benefit from renaming @Case to @Element.

Perhaps another issue at hand here is the discoverability of @Introspect?

[Pante]

I created a rough prototype here. Do let me know your thoughts!

@Case was renamed to @Label. One important difference is that all labels must now have a unique value. This differs from @Case where a value was neither required nor unique. Additionally, a new group attribute was introduced to @Label. This allows us to retrieve all @Labels in a group. Groups are expected to tie-in nicely with support for parameterized testing further down the line.

Cases have been reworked into Labels. Accessing values by index has always been error-prone and hence no longer supported.
A few nice QOL changes have been made to it, including the addition of single(). I'm not too sure what other ergonomic changes are missing. Feedback on it is greatly appreciated. In general, I tried to avoid anything related to sequencing/ordering since the order of elements is highly likely to be subject to frequent changes.

[CC007]

Perhaps another issue at hand here is the discoverability of @Introspect?

Ah while I had seen @Introspect, I hadn't looked into what it does. Most of my knowledge comes from this article.

Looking at the wiki, I see that it is described there. The only thing I'd add there is a code example, but other than that it's fine.

But taking into account @Introspect, I agree that @Element isn't a good name due to the naming conflict and that @Label is the better choice here.

I like the label changes

[CC007]

Also, you could use the TestAbortedException (which is also used by Junit Jupiter uses for test assumptions), instead of the IllegalStateExceptions

[CC007]

Fair. While I believe that the dependency is included with Junit Jupiter, it is always good to not rely on transitive dependencies and to use as few dependencies as possible when writing a library, to prevent dependency conflicts, so I agree with that choice.

[toolforger]

If possible(!), keep the old annotations and Javadoc them as @deprecated, explaining how to migrate to the new system.

[Pante]

We're planning to include this in 2.0.0, which probably will be a clean break.

[CC007]

In that case it would be nice if you added @Deprecated(forRemoval=true) in version 1.2.0

[Pante]

I thought a little further about it and I think I'm going to leave only the @Case in 2.0.0 marked as deprecated for removal. It'll point to a migration doc that I'll be writing. If only we had the tooling to automatically migrate it over to the new annotation :(.

[toolforger]

You could write an annotation processor that generates a file with updated code for copy&paste ;-)
Not sure if this is a good idea or Just Too Much Work, just an idea that popped up on my mind.

More seriously, Elementary could simply continue supporting @Case.
The typical behaviour for long-lived code is to keep such stuff until the next major revision (that would be 3.x).
Stuff in really wide-spread use (such as Java itself, or Maven) actually keeps it indefinitely.
(Gradle and MongoDB are exceptions, and people heavily dislike losing features, even wen it's a major revision.)

[Pante]

I really want to create an automated tool to do it but I just can't justify the effort for it sadly.

Thinking about it, I would honestly prefer a clean break between major version. My thought process behind it is that it's less maintenance burden. Also (to my knowledge at least), Elementary isn't widely used and I think it's fine and we should take advantage while we can to make breaking changes. People can always stay on 1.X

[Pante]

I opened a PR outlining all the changes to @Case and related APIs. If there's no issues with it, I'll be merging it in during the weekends.

[Pante]

Fixed in #164