Review for RFR 7902119: Add JemmySupport to jemmy/v3
Sergey Grinev
sergeygrinev at mail.ru
Sun Dec 2 22:03:36 UTC 2018
Hi Shura.
it's very useful and well written library. Docks are great addition to
Jemmy toolkit!
I have only few minor commens as follows:
*0) Overview*
- Documentation is scarce
- There is only one test (although, pretty creative)
- Test infrastructure seems to use Jemmy v3 version not published to repo
- There are several deprecated classes
*1) Proccessor.java *
1.1) Typos
- Proccessor instead of Processor (or is to differ from
javax.annotation.processing.Processor? -- in this case, it can be
JemmyProcessor or DocksProcessor)
- onlyOnlce instead of onlyOnce (or even better, isAlreadyProcessed )
1.2) options field and overriding getSupportedOptions
AFAIK, these are redundant and SupportedOptions annotations can be used
instead
@SupportedOptions(Proccessor.ACTIONS)
1.3) same for types and getSupportedAnnotationTypes(), although here
it's questionable as you need to expose hardcoded package name.
@SupportedAnnotationTypes("org.jemmy.control.ControlType")
1.4) findAnnotation methods looks a bit ineffective
1.5) BooleanValueGetter (and its type-counterparts) will look better as
an instance inside BooleanArrayValueGetter to avoid exposure and recreation
*2) ControlSupport.java*
2.1) linkSuperClasses() method, line controls.add(root);
you are adding null to the list in case foundOne==true -- it seems root
variable assigning is missing there
2.2) line 462 "do a quicksort, at least" comment -- also you can store
ControlSupport in SortedList with corresponding comparator; it will help
in DockGenerator.java:82 too;
2.3) You are using two different ways to compare types during finding
superclass in ControlSupport.linkSuperClasses and
DockGenerator.generate() (line 82)
it seems worth to add a shared utility method
*3) DockGenerator.java*
3.1) there is no null check for superDockName in generate() method. I've
assumed it's due to prior verification in
ControlSupport.linkSuperClasses() (line 491), but for future stability
I'd add one
3.2) Given that $SUPERDOCK$ is a valid java identifier, placeholders
looks a bit unsafe.
3.3) parameter names "docks" looks misleading in constructor
public DockGenerator(ProcessingEnvironment processingEnv,
List<ControlSupport> docks)
3.4) line 357: are you sure it's a warning and not a showstopper? E.g.
file writing failures will lead to missing docks silently
*4) DumpGenerator.java*
4.1) all constants can be private
4.2) generate(): TransformerConfigurationException is excessive, it
extends already listed TransformerException
4.3) toStringDollar() utility method can be moved to DumpGenerator from
Proccessor class and made private, it's used only here
-- Sergey
> Hi,
>
> Could you kindly take a look on this integration. Along with the code I managed to introduce some minimal tests, because of which additional step is taken during test execution. Here is the sequence of steps taken when "ant test"n:
> 1. Source is compiled (from src dir)
> 2. Test data (from test_data dir) is compiled (into build/test_data) with Proccessor class as an annotation processor. These classes include a few Wrap class extensions and contain proper annotations. Generated sources are placed into build/test_docks.
> 3. The generated source (from build/test_docks) is then also compiled (into build/test_data).
> 4. Tests are compiled and run with build/test_data in class path.
>
> Webrev:http://cr.openjdk.java.net/~shurailine/7902119/webrev.00/
> Bug:https://bugs.openjdk.java.net/browse/CODETOOLS-7902119
>
> Thank you.
>
> Shura
More information about the jemmy-dev
mailing list