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