Review request: JDK-8004928 TEST_BUG: Reduce dependence of CoreLib tests from the AWT subsystem (ver. 2)
alexey.utkin at oracle.com
Thu Dec 13 11:48:47 UTC 2012
Here is new version fix:
On 12.12.2012 22:59, Mandy Chung wrote:
> On 12/12/12 7:41 AM, Alexey Utkin wrote:
>> Bug description:
>> The set of non-public interfaces was changed to avoid AWT
> 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.
>> Swing-stored constants were changed to locally-defined.
> 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.
>> 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.
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 , 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
> 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.
Thanks for review!
More information about the core-libs-dev