Review request for type annotation reflection test

[Alex Buckley]

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
>>>
>>
>