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
