Review request: JDK-8004928 TEST_BUG: Reduce dependence of CoreLib tests from the AWT subsystem (ver. 2)

[Alexey Utkin]

Here is new version fix:
     http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-8004928/webrev.02

On 12.12.2012 22:59, Mandy Chung wrote:
> On 12/12/12 7:41 AM, Alexey Utkin wrote:
>> Bug description:
>>     https://jbs.oracle.com/bugs/browse/JDK-8004928
>>
>
>> Summary:
>> *test/java/io/Serializable/resolveProxyClass/NonPublicInterface.java*
>> *test/java/lang/reflect/Proxy/ClassRestrictions.java*
>>         The set of non-public interfaces was changed to avoid AWT 
>> dependencies.
>>
> These 2 tests look for the first non-public system interface.  Instead 
> of keeping the nonPublicInterfaces list, it may be good to simplify 
> this to use "java.util.zip.ZipConstants" which seems to be a stable 
> one that will unlikely be removed.  I would suggest to add a check 
> that the class is non-public (Modifier.isPublic) to catch any future 
> change to this class.
Done.

>>         Swing-stored constants were changed to locally-defined.
>>
> *test/java/util/Collections/EmptyIterator.java*
>
> I believe your change captures what it's intended to test (typed 
> enumeration and rawtype enumeration).  Minor comment - it might be 
> better to move the new static variables as local variable before 
> calling testEmptyEnumeration to be consistent with convention used in 
> this test.  There is no need to keep them static anyway.   I would 
> also take out the comment about the substitution since this test 
> doesn't seem to have any relationship with javax.swing.
>
Done.

>> *test/java/util/logging/LoggingDeadlock4.java*
>>         Test case was simplified to avoid AWT class loading. Negative 
>> test result was tested on early
>>         JDK7 build.
>>
>
> Looks good.  Nit: L46 - good to break it into 2 lines.  It's good that 
> you have verified with and without the fix.
Done.

On 12.12.2012 22:40, Alan Bateman wrote:
> For java/io/Serializable/resolveProxyClass/NonPublicInterface.java and 
> java/lang/reflect/Proxy/ClassRestrictions.java then it would be nice 
> if the types used were in compact1 [1], that would avoid needing to 
> exclude those tests. I also see the test uses 
> sun.tools.agent.StepConstants which I don't think exists but perhaps 
> that is intentional.
The list was reduces to "java.util.zip.ZipConstants" (from compact1 
profile) with "non-public" status verification.

> test/java/util/Collections/EmptyIterator.java, minor nit but I think 
> we prefer "public static" over "static public". It doesn't of course 
> need to be public anyway.
>
The variables were made local-final in accordance with Mandy's 
recommendation.

> You probably saw Dan's comment about changing 
> test/java/util/logging/LoggingDeadlock4.java, I trust you'll double 
> check this test with an older version of the JDK that doesn't have the 
> fix. My only comment is that line 46 is too wide.
>
Done.

Thanks for review!
-uta