Add AnnotationValidator, and validation checks for Category (fix for issue #93) #684
Conversation
| @@ -1,5 +1,8 @@ | |||
| package org.junit.experimental.categories; | |||
|
|
|||
| import org.junit.experimental.validator.CategoryValidator; | |||
| import org.junit.experimental.validator.Validator; | |||
There was a problem hiding this comment.
Thinking out loud. Would ValidateWith be more consistent? Curious for your opinion before we make any change.
There was a problem hiding this comment.
I actually do prefer ValidateWith, it is a bit more specific. Should I change it?
|
Great start. As I note, the primary concern is trying to make sure that we keep the number of annotation lookups and reflective calls down to a minimum, since some platforms have performance that is easily impacted by such operations. With the right caching, we'll be really close. |
|
I'm really sorry this seems to have dropped off my radar. Taking a look. |
| import org.junit.runners.model.RunnerScheduler; | ||
| import org.junit.runners.model.Statement; | ||
| import org.junit.runners.model.TestClass; | ||
| import org.junit.runners.model.*; |
|
Big step in the right direction, thanks, and sorry again for dropping the ball on the review. |
|
Made the changes as requested. No dramas about forgetting about the change, I kind of forgot about it too. Thanks for helping out. |
|
@diusricky FYI: this pull cannot be automatically merged. You might want to pull from the main repo, merge the branch, and update the pull request. |
| import java.util.List; | ||
|
|
||
| /** | ||
| * Provides an interface with callback methods to validate annotations. |
There was a problem hiding this comment.
Perhaps simply "Validates annotations on classes and methods. To be validated, an annotation should be annotated with {@link ValidateWith}
There was a problem hiding this comment.
@codingricky I don't see the above change. Do you still have changes to push to your public repo?
|
|
||
| /** | ||
| * Gets a {@code Map} between annotations and fields that have | ||
| * the annotation in this class or its superclasses. |
There was a problem hiding this comment.
This should have an @since 4.12 tag
|
@kcooney merging is fine. But I would like to make some additional pull requests before we release JUnit 4.12. |
| /** | ||
| * Miscellaneous functions dealing with {@code Throwable}. | ||
| * | ||
| * @author kcooney@google.com (Kevin Cooney) |
There was a problem hiding this comment.
Any idea why this file of Kevin's is sneaking into your pull request?
There was a problem hiding this comment.
@codingricky You can solve this problem with git rebase master.
There was a problem hiding this comment.
Thanks guys, fixed.
On 08/10/2013, at 8:03 AM, David Saff notifications@github.com wrote:
In src/main/java/org/junit/internal/Throwables.java:
@@ -0,0 +1,42 @@
+package org.junit.internal;
+
+/**
- * Miscellaneous functions dealing with {@code Throwable}.
- * @author kcooney@google.com (Kevin Cooney)
Any idea why this file of Kevin's is sneaking into your pull request?—
Reply to this email directly or view it on GitHub.
- Fixed indentation - Simplified hasValidatorAnnotation method - Deleted test that was covered elsewhere
- extracted creation of annotation validator into a factory - added thread saftey around collections
Moved validator to category packages. Changes around unmodifiable maps. Made AnnotationValidator abstract.
- ParentRunner now has null object pattern for null annotation validators - unmodifiableLists are now inserted into the maps of fMethodsForAnnotations and fFieldsForAnnotations - null value check has been added to the AnnotationValidatorFactory
- Fixed javadoc comment in map getAnnotationToMethods/getAnnotationToFields methods - Fixed duplicate test for annotationToMethods
…Class - Removed redundant wrapping of unmodifiableMap in getters of annotationToFields/Methods
- Fixed up some imports
…lidators - added a comment regarding sorting of declared fields - iterated over map using entry sets and not key sets in TestClass
- refactored the construction of the incompatiable annotations of CategoryValidator to be initialized immediately
… to java.lang.reflect
|
Looks in pretty good shape to me. @kcooney, did you have any remaining questions? |
|
Merging! Thanks for the patient work, @codingricky. @kcooney, if any additional concerns come up, let's handle them in a follow-up pull. @codingricky, can you add a note about this new feature at https://github.com/junit-team/junit/wiki/4.12-release-notes? Thanks! |
|
Done! |
Add AnnotationValidator, and validation checks for Category (fix for issue #93)
|
Actually merging. :-P Not sure what happened the first time. Thanks again! |
Any feedback would be greatly appreciated. Thanks!