Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make core features of AnnotationUtils available to extension authors #246

Closed
nipafx opened this issue May 7, 2016 · 9 comments
Closed

Comments

@nipafx
Copy link
Contributor

nipafx commented May 7, 2016

While implementing extensions it is common to look for annotations. The easiest way to do this properly (i.e. including repeatable and meta-annotations) seems to be AnnotationUtils. It is currently marked as internal API.

Annotations and especially meta-annotations are a pillar of JUnit 5's extensibility. Being able to freely compose existing annotations according to ones own requirements will be a great boost for customizations. It would be counter-intuitive if third-party extensions would not compose as well. They are hence required to use the same look-up logic as official annotations.

The easiest way to do this is to encourage using the same utility to find annotations.

@sbrannen
Copy link
Member

sbrannen commented May 7, 2016

@nicolaiparlog, thanks for raising this issue. You make some valid arguments.

@junit-team/junit-lambda, let's discuss the options here.

@sbrannen sbrannen added this to the 5.0 Backlog milestone May 7, 2016
@sbrannen sbrannen changed the title Make AnnotationUtils public API Make AnnotationUtils a public API May 7, 2016
@smoyer64
Copy link
Contributor

smoyer64 commented May 7, 2016

I'm working on an extension (https://github.com/selesy/uprest) to replace the defunct Restfuse project (https://github.com/eclipsesource/restfuse). Since the tests are defined declaratively, I make heavy use of annotations and also ended up writing a small utility class to retrieve annotation values. I can potentially have:

  • Annotations on the type
  • Annotations on a method
  • Annotations on a parameter to be resolved with a MethodParameterResolver e.g.
@Paths("/scim/v2")
public ResourceTypesTest

  @Paths("/ResourceTypes")
  @Methods({Method.DELETE, Method.PATH, Method.POST, Method.PUT})
  @Headers({
    "Content-Type:application/scim+json",
    "Accept:application/scim+json"
  })
  public void test(@EntityBody String entityBody, @Header("Content-Type") String contentType) { ... }

And now I'm realizing that to make the test code well-factored, I also need to detect annotations up the parent type hierarchy as well as to the super() of each method (if they're overriden).

I guess that's a long-winded way of saying +1!

@marcphilipp
Copy link
Member

@smoyer64 upREST looks really promising! Great stuff!

I quite agree that we should make this functionality available to extensions. However, I'm not convinced that making AnnotationUtils a public API is the way to go. An alternative would be to add additional methods to ExtensionContext, e.g. getAnnotations(Class<A extends Annotation>). The benefit would be more control over what is public API and what isn't. If we made parts of AnnotationUtils public API I'm pretty sure all of it will end up being used and we would have problems fixing even small bugs in it because it might break an extension.

@sbrannen
Copy link
Member

sbrannen commented May 7, 2016

I agree with @marcphilipp and actually already had the exact same idea... to provide a public API local to the ExtensionContext.

@smoyer64
Copy link
Contributor

smoyer64 commented May 7, 2016

Either way is fine with me ... and I certainly know the value of limited the surface area of the actual API (leaving more code as the proverbial "black box"). I think the spirit of the proposal by @nicolaiparlog would be met by exposing that functionality elsewhere.

As an aside, since (at the moment) the upREST framework's two primary goals are to create dynamic tests (the example above would actually produce four tests) and to execute the tests having already sent the HTTP request and received the HTTP response, most of the work is done behind the scenes as a means of satisfying one or more MethodParameterResolvers. This involves a chaining of dependent resolvers where everything is dependent upon resolving the HTTP request even though many times it's not "injected" into the method call.

In any case, I guess what I'm really looking for is a little guidance on when using the ExtensionContext is more appropriate than the MethodInvocationContext.

I'd also like to extend the use of the JUnit5 @API tag to the upREST extension(s). Assuming JUnit5 users learn to "honor" the status of the API (using only maintained?), it seems like they'd understand or even expect the same from extensions.

@nipafx
Copy link
Contributor Author

nipafx commented May 7, 2016

My intent was to gain supported access to the functionality and I am happy that you are seeing things similarly. :) I don't care much which way it happens and absolutely see your point to concentrate API access via execution contexts instead of simply "making stuff public".

@mmerdes
Copy link
Contributor

mmerdes commented May 9, 2016

I agree with @marcphilipp and @sbrannen that it might be wise to hide the implementation of the annotation utils properly. We also might want to expose the functionality in an adequate way to prevent people from reinventing the wheel or even worse from using internal stuff reflectively...

@smoyer64 first of all, thanks for working on such an interesting application of JUnit5 features.
I am looking forward to see and use the result.
A first implementation of dynamic tests is planned for M1 at the end of May.

@sbrannen sbrannen changed the title Make AnnotationUtils a public API Make core features of AnnotationUtils available to extension authors May 9, 2016
@sbrannen sbrannen modified the milestones: 5.0 M2, 5.0 Backlog May 9, 2016
@sbrannen sbrannen modified the milestones: 5.0 M2, 5.0 M3 Jul 15, 2016
@sbrannen sbrannen modified the milestones: 5.0 M3, 5.0 M4 Jul 25, 2016
@sormuras sormuras self-assigned this Feb 14, 2017
@sormuras
Copy link
Member

sormuras commented Feb 14, 2017

in progress - see draft PR #651

sormuras added a commit that referenced this issue Feb 15, 2017
Fixes #246 by publishing useful internal helper methods via an API
facade. At the moment, only annotation-related helpers are included.
The ExtensionContext.Util interface might grow in the future.
sormuras added a commit that referenced this issue Feb 15, 2017
Fixes #246 and #352 by publishing useful internal helper methods via an API
facade. The Utilities interface might grow in the future.
sormuras added a commit that referenced this issue Feb 15, 2017
Fixes #246 and #352 by publishing useful internal helper methods via an API
facade. The Utilities interface might grow in the future.
sormuras added a commit that referenced this issue Feb 16, 2017
Fixes #246 and #352 by publishing useful internal helper methods via an
API facade. The Utilities interface might grow in the future.
@marcphilipp
Copy link
Member

see #352 (comment)

sormuras added a commit that referenced this issue Feb 22, 2017
Fixes #246 and #352 by publishing useful internal helper methods via an
API facade. Members of the support package are maintained, which is
reflected by the `@API(Maintained)` annotation.
sormuras added a commit that referenced this issue Feb 22, 2017
Fixes #246 and #352 by publishing useful internal helper methods via an
API facade. Members of the support package are maintained, which is
reflected by the `@API(Maintained)` annotation.
sormuras added a commit that referenced this issue Feb 22, 2017
Fixes #246 and #352 by publishing useful internal helper methods via an
API facade. Members of the support package are maintained, which is
reflected by the `@API(Maintained)` annotation.
sormuras added a commit that referenced this issue Feb 22, 2017
Fixes #246 and #352 by publishing useful internal helper methods via an
API facade. Members of the support package are maintained, which is
reflected by the `@API(Maintained)` annotation.
sormuras added a commit that referenced this issue Feb 22, 2017
Fixes #246 and #352 by publishing useful internal helper methods via an
API facade. Members of the support package are maintained, which is
reflected by the `@API(Maintained)` annotation.
sormuras added a commit that referenced this issue Feb 22, 2017
Fixes #246 and #352 by publishing useful internal helper methods via an
API facade. Members of the support package are maintained, which is
reflected by the `@API(Maintained)` annotation. The current support
facade only delegates to the internal utility methods.
sormuras added a commit that referenced this issue Feb 23, 2017
Fixes #246 and #352 by publishing useful internal helper methods via an
API facade. Members of the support package are maintained, which is
reflected by the `@API(Maintained)` annotation. The current support
facade only delegates to the internal utility methods.
sormuras added a commit that referenced this issue Feb 23, 2017
Fixes #246 and #352 by publishing useful internal helper methods via an
API facade. Members of the support package are maintained, which is
reflected by the `@API(Maintained)` annotation. The current support
facade only delegates to the internal utility methods.
sormuras added a commit that referenced this issue Feb 23, 2017
Fixes #246 and #352 by publishing useful internal helper methods via an
API facade. Members of the support package are maintained, which is
reflected by the `@API(Maintained)` annotation. The current support
facade only delegates to the internal utility methods.
sormuras added a commit that referenced this issue Feb 23, 2017
Fixes #246 and #352 by publishing useful internal helper methods via an
API facade. Members of the support package are maintained, which is
reflected by the `@API(Maintained)` annotation. The current support
facade only delegates to the internal utility methods.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants