Review for RFR 7902119: Add JemmySupport to jemmy/v3

[Alexandre (Shura) Iline]

Thank you very much for the review, Sergey!

I will fix those problems which I think have to be fixed before the initial push - those which may create potential inconsistencies in the future.

For the rest, I hope you are OK if I create bugs to track. It is only the initial push, after all.

Shura

> On Dec 2, 2018, at 2:03 PM, Sergey Grinev <sergeygrinev at mail.ru> wrote:
> 
> 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
>