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
