Question about the bug https://bugs.openjdk.java.net/browse/JDK-8031179

[Eric Wang]

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
>>>>
>>>
>>
>