-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[Spring] Support @CucumberContextConfiguration as a meta-annotation #2630
[Spring] Support @CucumberContextConfiguration as a meta-annotation #2630
Conversation
…tion Using @CucumberContextConfiguration as a meta-annotation caused a CucumberBackendException because SpringFactory only realized raw use of the class. Method hasCucumberContextConfiguration does now recognize the use of @CucumberContextConfiguration as meta-annotation or with inheritance. Also SpringBackend#loadGlue filters out abstract classes and interfaces to not try to instantiate what cannot be instantiated.
Codecov Report
@@ Coverage Diff @@
## main #2630 +/- ##
============================================
- Coverage 84.79% 84.77% -0.03%
+ Complexity 2693 2686 -7
============================================
Files 322 322
Lines 9544 9530 -14
Branches 908 903 -5
============================================
- Hits 8093 8079 -14
Misses 1120 1120
Partials 331 331
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers! Looks good in principle, but I've got some style remarks and questions.
Also don't forget to update the CHANGELOG.md
.
cucumber-spring/src/test/java/io/cucumber/spring/cucontextconfig/WithMetaAnnotation.java
Outdated
Show resolved
Hide resolved
cucumber-spring/src/main/java/io/cucumber/spring/SpringFactory.java
Outdated
Show resolved
Hide resolved
cucumber-spring/src/main/java/io/cucumber/spring/SpringFactory.java
Outdated
Show resolved
Hide resolved
...spring/src/test/java/io/cucumber/spring/cucontextconfig/AbstractWithComponentAnnotation.java
Outdated
Show resolved
Hide resolved
cucumber-spring/src/test/java/io/cucumber/spring/SpringFactoryTest.java
Outdated
Show resolved
Hide resolved
Wow, thanks for your prompt reply and all your remarks and suggestions! I wasn't able to fix all points today, but I will see that I can do another commit tomorrow. I will also look into the AnnotatedElementUtils' docs again and add some more tests to clarify. |
…tion Adds suggestions from review: renames test package, more focused testing and updates the changelog. Method hasCucumberContextConfiguration now uses AnnotatedElementUtils isAnnotated with considers all merged annotations (including inherited and meta-annotation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Do consider adding a note to the javadoc of CucumberContextConfiguration
to note that the annotation can be used as a meta annotation. Please also check the surrounding code for error messages (e.g. checkOnlyOneClassHasCucumberContextConfiguration
) to see if it need to mention (meta-)annotation
.
Hi @m-schlatt, Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾 In return for this generous offer we hope you will:
On behalf of the Cucumber core team, |
@m-schlatt cheers, I've got ahead and merged and released the change. |
Using @CucumberContextConfiguration as a meta-annotation caused a CucumberBackendException because SpringFactory only realized raw use of the class. Method hasCucumberContextConfiguration does now recognize the use of @CucumberContextConfiguration as meta-annotation or with inheritance.
Also SpringBackend#loadGlue filters out abstract classes and interfaces to not try to instantiate what cannot be instantiated.
🤔 What's changed?
Using @CucumberContextConfiguration as a meta-annotation caused a CucumberBackendException because SpringFactory only realized raw use of the class. the method SpringFactory#hasCucumberContextConfiguration does now recognize the use of @CucumberContextConfiguration as meta-annotation or frominheritance.
Also SpringBackend#loadGlue filters out abstract classes and interfaces to not try to instantiate what cannot be instantiated.
⚡️ What's your motivation?
Fixes: #2491
🏷️ What kind of change is this?
♻️ Anything particular you want feedback on?
Did I get the suggested changes right? Also code fit and quality
📋 Checklist:
This text was originally generated from a template, then edited by hand. You can modify the template here.