Hi Stuart,
Please review the webrev
http://cr.openjdk.java.net/~ewang/JDK-8031179/webrev.00/, if you are OK
with the changes, could you please be my sponsor?
Thanks,
Eric
On 2014/1/24 15:14, Eric Wang wrote:
Hi Stuart,
Thanks for the suggestion! sorry for reply this mail late as i was
busy on other tasks
The webrev has been in the internal review process. Based on the
suggestion, here is a summary of changes:
1. Add othervm options to tests:
java/rmi/Naming/DefaultRegistryPort.java
java/rmi/Naming/legalRegistryNames/LegalRegistryNames.java
java/rmi/Naming/LookupNameWithColon.java
java/rmi/server/UnicastRemoteObject/exportObject/GcDuringExport.java
sun/rmi/rmic/RMIGenerator/RmicDefault.java
java/rmi/MarshalledObject/compare/Compare.java
java/rmi/MarshalledObject/compare/HashCode.java
2. Remove java/rmi and sun/rmi from othervm.dirs of TEST.ROOT
3. Filed a another bug
https://bugs.openjdk.java.net/browse/JDK-8032629 for exclusiveAccess.dirs
Thanks,
Eric
On 2014/1/18 8:45, Stuart Marks wrote:
Hi Eric,
Thanks for doing the analysis of the tests that need /othervm. The
list of tests that don't need /othervm looks good. One subtlety is
this one:
java/rmi/activation/ActivationGroupDesc/checkDefaultGroupName/CheckDefaultGroupName.java
Most activation tests do need /othervm, but this one doesn't, since
all it does is create an ActivationGroupDesc instance, which has no
side effects. This is unusual for the activation APIs, since most of
them are quite intertwined with the rest of the activation subsystem.
So, for this test, the lack of /othervm warrants a comment explaining
why /othervm is unnecessary.
Regarding merging of the tests
java/rmi/MarshalledObject/compare/Compare.java and HashCode.java,
this is fairly subtle and not obviously wrong, but it's not obviously
right either. Note that different jtreg tests -- even in agentvm or
samevm mode -- load the test class in a fresh classloader, which
means that the statics are reinitialized. This provides some degree
of isolation of the tests even when they're reusing the same JVM. By
contrast, calling the two different methods from within the same test
exposes the second one to initializations left from the first one. In
addition (though I'm not too concerned about this) if the first test
fails the second won't be run at all. Merging these tests is kind of
out of scope for this particular bug, and since I wasn't fully able
to convince myself that the merged tests have the same semantics as
the unmerged tests, I'd prefer not to see the merging of these tests
as part of this changeset.
(Some cleanup of these two tests is probably warranted eventually.
I'd take a different approach of merging and refactoring the sources
but keeping them as separate test runs, using two @run tags. But we
should work on that separately. Meanwhile, the overhead of having
these as separate tests is minimal, especially if they're not run in
/othervm mode.)
I don't think the removal of java/rmi/Naming from
exclusiveAccess.dirs is safe at this point. The
DefaultRegistryPort.java test uses the default registry port, not a
unique one. Conceptually it would need to be converted to use the
TestLibrary unique port stuff, but the test is actually about using
the default port. So, the solution here isn't obvious.
In addition, the
java/rmi/Naming/legalRegistryNames/LegalRegistryNames.java test also
uses the default registry port. It too will need to converted before
java/rmi/Naming is removed from exclusiveAccess.dirs.
The java/rmi/Naming/LookupIPv6.java test was converted to use the
TestLibrary unique port technique, but only partially. The registry
is created on the unique port, but the Naming.lookup() call on the
last line of the test doesn't provide a port number, so it does the
lookup on the default port instead. This will cause the test to fail
in almost all cases.
I have to ask, did you run this test with your modifications?
(Well, the test would pass if IPv6 is not enabled on the machine
running the test, but it only passes because the part of the test
that creates and uses the registry is bypassed entirely if IPv6 is
not enabled. If you're modifying code, you need to take
responsibility for ensuring that the code being modified is actually
being run and is doing what you expect.)
LookupIPv6.java also needs to have these lines added to the test tags:
* @bug 4402708
+ * @library ../testlibrary
+ * @build TestLibrary
* @run main/othervm -Djava.net.preferIPv6Addresses=true LookupIPv6
(Their absence will cause the test also to fail in a clean build, but
the test will find a TestLibrary class if one had been compiled by a
previous test that had required it.)
Maybe we should separate the othervm changes (removal of the rmi dirs
from othervm.dirs, and addition of /othervm) from the
exclusiveAccess.dirs changes. A separate bug would be filed for
exclusiveAccess.dirs. I know this means the bug count won't go down,
but dealing with exclusiveAccess.dirs means that the logic of various
tests will need to be change to use the unique port mechanism. This
is a fair chunk of work and it's logically distinct from the othervm
work.
If we also remove the merging of the MarshalledObject tests, I think
we can proceed with the othervm changes.
Eric, can you make these changes and send out another webrev?
Thanks.
s'marks
On 1/16/14 7:18 AM, Eric Wang wrote:
Hi Stuart,
I have generated a webrev for review, can you please help to check
whether the changes are OK.
http://cr.openjdk.java.net/~ewang/JDK-8031179/webrev.00/
<http://cr.openjdk.java.net/%7Eewang/JDK-8031179/webrev.00/>
Also need your help to confirm that the rest 9 tests in previous
mail don't need the /othervm option indeed.
Thanks,
Eric
On 2014/1/16 22:55, Eric Wang wrote:
Hi Stuart,
I'm working on the bug
https://bugs.openjdk.java.net/browse/JDK-8031179 to add /othervm
option to rmi tests, so they can removed from the item othervm.dir
and exclusiveAccess.dirs.
By searching the directories java/rmi, sun/rmi and java/rmi/Naming,
there are 14 tests without othervm option, but only 5 tests need to
the /othervm tag. they are:
Tests need /othervm option:
java/rmi/Naming/DefaultRegistryPort.java
java/rmi/Naming/legalRegistryNames/LegalRegistryNames.java
java/rmi/Naming/LookupNameWithColon.java
java/rmi/server/UnicastRemoteObject/exportObject/GcDuringExport.java
sun/rmi/rmic/RMIGenerator/RmicDefault.java
Tests don't need the /othervm option:
java/rmi/activation/ActivationGroupDesc/checkDefaultGroupName/CheckDefaultGroupName.java
java/rmi/MarshalledObject/compare/Compare.java
java/rmi/MarshalledObject/compare/HashCode.java
java/rmi/MarshalledObject/compare/NullReference.java
java/rmi/server/Unmarshal/PrimitiveClasses.java
sun/rmi/log/ReliableLog/LogAlignmentTest.java
sun/rmi/log/ReliableLog/Recovery.java
sun/rmi/log/ReliableLog/SnapshotSize.java
sun/rmi/rmic/classpath/RMICClassPathTest.java
Also i have a bit confuse about the test Compare.java and
HashCode.java in java/rmi/MarshalledObject/compare directory, as
they should be merged into one test so that we don't need to add
additional /othervm option for 2 test.
Thanks,
Eric