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
