Review request for type annotation reflection test

[Charlie Wang]

All right. Got it. Thank you all them same for your previous quick and 
constant support.



- Charlie

On 2013/8/29 9:14, Alex Buckley wrote:
> Charlie,
>
> I can't devote any more time to these tests. Sergey and I have given 
> multiple rounds of detailed feedback on the architecture of your test 
> framework and on individual lines of code. You own these tests; it is 
> your responsibility to take feedback and improve them.
>
> Alex
>
> On 8/28/2013 5:25 PM, Charlie Wang wrote:
>> Hi Alex,
>>    Could you take a look at the changes I agreed to make and let me know
>> if you are OK with them?
>>
>>
>>
>> - Charlie
>>
>> On 2013/8/26 15:45, Charlie Wang wrote:
>>> Hi Alex,
>>>   I think I will need to make update according to Sergey's comments
>>> before check in the code.
>>>
>>> Sergey,
>>>   Thanks for the comments. My reply inline.
>>>
>>> On 2013/8/23 18:36, Sergey Grinev wrote:
>>>> Hi, all.
>>>>
>>>> Sorry if this review covers same issue I reported on reviewboard one,
>>>> but it seems few important point were not fixed:
>>>>
>>>> 1.  public Map<Helper.Declaration, Object> lhm = new 
>>>> LinkedHashMap<>();
>>>> and
>>>>     public synchronized static String genDeclaration(
>>>>             Map<Helper.Declaration, Object> input, Declaration...
>>>> srcType)
>>>>
>>>> - fields used as a method-local temporary variables are error-prone
>>>> and confusing. Users/readers of that code are not aware when this
>>>> field can be used or when this field can be cleared, also using such
>>>> field is not thread-safe. Imagine someone added one more ENUM value
>>>> to TestCase, used lhm and didn't call lhm.clear() -- things will
>>>> break and very different place this person didn't even touch.
>>>>
>>>> - Map claims it stores Objects but actually they are only String and
>>>> String[]. Thus your getDeclaration() has invalid signature and is
>>>> error-prone. Also you calls instanceof several times to resolve this
>>>> ambiguity which is considered to be an anti-pattern as well.
>>>>
>>>> You can solve these problems by introducing separate entity
>>>> responsible for declaration generation:
>>>>
>>>> public class DeclarationGenerator {
>>>>
>>>>     private final Map<Helper.Declaration, String[]> lhm = new
>>>> LinkedHashMap<>();
>>>>         public void add(Helper.Declaration key, String... value) {
>>>>         lhm.put(key, value);
>>>>     }
>>>>     public String genDeclaration(Declaration... srcType) {
>>>>         //...
>>>>         for (Declaration st : Declaration.values()) {
>>>>                 if (input.containsKey(st))
>>>> result.append(st.getSrc(input.get(st)));
>>>>         }
>>>>         //...
>>>>     }
>>>> }
>>>>
>>>> with this helper class instead of
>>>>
>>>>                 lhm.clear();
>>>>                 lhm.put(Helper.Declaration.IMPORT, null);
>>>>                 lhm.put(Helper.Declaration.CLASS, str[0]);
>>>>                 testCode += Helper.genDeclaration(lhm);
>>>>
>>>> you can write
>>>>
>>>>                 DeclarationGenerator dg = new DeclarationGenerator();
>>>> // not to reuse, create new one for each generation
>>>>                 dg.put(Helper.Declaration.IMPORT, null);
>>>>                 dg.put(Helper.Declaration.CLASS, str[0]);
>>>>                 testCode += dg.genDeclaration();
>>>>
>>>> having clean design without field used as temporary variable and type
>>>> ambiguity.
>>>>
>>> I agree with you. I will update the code accordingly.
>>>
>>>>
>>>> 2. several TestCase enum entries, especially TESTBASECLSSTART and
>>>> TESTBASECLS are copypasted in several places.
>>> I will remove the duplications.
>>>
>>>>
>>>> 3. public static Map<String, String> testInput = new 
>>>> LinkedHashMap<>();
>>>>
>>>> why this field in static?
>>>>
>>> Made it static for enum methods to access directly.
>>>
>>>
>>> And as to the other comments from you:
>>>
>>>> 1. Helper#getAnno()
>>>>
>>>>   StringBuffer result = new StringBuffer("");
>>>>
>>>> using StringBuilder is recommended for thread-safe methods
>>> Agree.
>>>
>>>>
>>>> 2. List Helper.getMultipleAT()
>>>>
>>>> this method uses List where List<String> can be specified
>>> Good check.
>>>
>>>>
>>>> 3. Helper.compareAnnoWithDiffSeq()
>>>>
>>>> this can be done much shorted and without copying array to list:
>>>>
>>>>         String[] split1 = anno1.trim().split("\\s+");
>>>>         String[] split2 = anno2.trim().split("\\s+");
>>>>         Arrays.sort(split1);
>>>>         Arrays.sort(split2);
>>>>         return Arrays.equals(split1, split2);
>>> OK.
>>>
>>>>
>>>> 4. Helper.compileCode():697-704 can be simplified using jdk8 streams:
>>>>
>>>> ok = diagnostics.getDiagnostics().stream().anyMatch(d -> d.getKind()
>>>> == Diagnostic.Kind.ERROR);
>>> I didn't adopt it because I wasn't sure if I supposed to use this new
>>> jdk8 feature in this jdk8 test. Bus since Tristan also suggested it is
>>> doable, I will update my code this way.
>>>
>>>>
>>>> 5. Same for Helper.isPkgInfoPresent:
>>>>
>>>> return files.stream().map(JavaFileObject::getName).anyMatch(name ->
>>>> name.contains("package-info") || name.contains("PkgAnno"));
>>> OK.
>>>
>>>>
>>>> 6. AnnotatedArrayTypeGetAnnotatedGenericComponentTypeTest:175
>>>>
>>>> You are calling Helper.getArrAT(at) then split it by ";" and work
>>>> with result.
>>>> while Helper.getArrAT() takes a list and convert it into string:
>>>> return getAnno(annotations) + ";" + ret;
>>>>
>>>> It may be more efficient to work directly with lists.
>>> The expected results (to be compared with test results) also come as
>>> strings because they are directly extracted from the test input, which
>>> is why I made it "getAnno(annotations) + ";" + ret" - so I don't have
>>> to split the result into lists. Either way, the conversion has to be
>>> made. The present way is more direct.
>>>
>>>>
>>>> 7. DeclarationGenerator interface is never used as interface
>>> I don't think it hurts to put an interface there. At least it will
>>> help extension.
>>>
>>>>
>>>> 8. Helper.Declaration.replaceAnnoStr should be static
>>> Isn't it static already.
>>>
>>>>
>>>> 9. Helper.Declaration.replaceAnnoStr/getFirstAnno return result as
>>>> String[] array, which is considered to be a bad practice (you are
>>>> using variable length structure to store exactly 2 values). It would
>>>> be cleaner to introduce intermediate structure of 2 string to store
>>>> return value.
>>> Then I will have to declare a new class for the two strings.
>>>
>>>
>>> I will update my code according to most of your comments. Could you
>>> please reply to me asap if you think there's anything missing.
>>> Hopefully we can wrap things up within the week.
>>>
>>>
>>>
>>> - Charlie
>>>
>>>
>>>>
>>>> On 22-Aug-13 22:43, Alex Buckley wrote:
>>>>> I have no further comments. Thanks for all your work to test this
>>>>> new and expansive API.
>>>>>
>>>>> Alex
>>>>>
>>>>> On 8/20/2013 1:56 AM, Charlie Wang wrote:
>>>>>> Hi Alex,
>>>>>>    I used testInput.size() instead of static int clsIdx as suffix 
>>>>>> this
>>>>>> time. And other update according to your comments.
>>>>>>
>>>>>> Here's the link. Please review again.
>>>>>> http://cr.openjdk.java.net/~pzhang/Charlie/TypeAnnotation1/webrev/
>>>>>>
>>>>>>
>>>>>> - Charlie
>>>>>>
>>>>>> On 2013/8/9 1:05, Alex Buckley wrote:
>>>>>>> - AnnotationTest.i/clsIdx: it wasn't the name of the variable I was
>>>>>>> objecting to. It was the fact that you're relying on a mutable 
>>>>>>> public
>>>>>>> static variable at all.
>>>>>>>
>>>>>>> - GenTestCode should take Class<? extends TestCaseGenerator>, so 
>>>>>>> you
>>>>>>> don't need to cast to TestCaseGenerator in GenTestCode. (Same 
>>>>>>> comment
>>>>>>> for AnnotationTest.compileCode).
>>>>>>>
>>>>>>> - AnnotationTest.testInput is a Map<Object,String> but I'm sure you
>>>>>>> can do better than Object for keys.'
>>>>>>>
>>>>>>> - ExecutableGetAnnotatedParameterTypesTest.testInput hides
>>>>>>> AnnotationTest.testInput, but swaps the order of the type 
>>>>>>> arguments!
>>>>>>> Map<String,Object> is either wrong, or should not use Object.
>>>>>>>
>>>>>>> Alex
>>>>
>>>
>>