-
Notifications
You must be signed in to change notification settings - Fork 12
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
feat(#199): Adds package annotation for affected tests #258
Conversation
074b51a
to
40b79b9
Compare
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.
Great stuff @lordofthejars. Few minor things you could fix in between the conferences ;)
|
||
} | ||
|
||
private ComponentUnderTest[] getAllAnnotations(JavaClass testJavaClass) { |
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.
Minor detail - fix it when you will find time - instead of generic getAllAnnotations
- findComponentsUnderTests
?
} | ||
} | ||
} | ||
|
||
private void addToIndex(JavaElement javaElement, String[] imports) { | ||
private List<String> calculateManualAddedDependencies(JavaClass testJavaClass) { |
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.
calculateManualAddedDependencies
-> calculateManuallyAddedDependencies
|
||
JavaAssistClass(CtClass classReference) { | ||
imports = findImports(classReference); | ||
className = classReference.getName(); | ||
this.classReference = classReference; |
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.
For consistency can we prefix all with this.
?
@Retention(RetentionPolicy.RUNTIME) | ||
@Target(ElementType.TYPE) | ||
@Documented | ||
public @interface ComponentsUnderTest { |
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.
There is no javadoc nor documentation description
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.
It's implicitly used for Java8+ as @repeatable annotation, and we don't really support any lower version of JDK right now
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.
We document the use of the main annotation though https://github.com/arquillian/smart-testing/pull/258/files#diff-10bc48a187b42a0137c1e00172ee76bfR8
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.
Yeah, I know. I just thought that it would be nice to have an example/doc of this annotation as well. But I don't really insist on it ;-)
Short description of what this resolves:
Adds package annotation for affected tests
Changes proposed in this pull request:
Adds
@ComponentUnderTest
annotation to set which production classes are affected by annotated tests. Useful for black box testing.Adds library to scan classes of given package. It is a really light library (no external dependencies), offers a lot of operations that we might use in the future. Also maybe it can be used as a substitution of Javassist.
Fixes #199