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

[Eric Wang]

Hi Stuart,

I have made changes based on your comments. Can you please review again? 
Thanks a lot!
http://cr.openjdk.java.net/~ewang/JDK-8031179/webrev.01/ 
<http://cr.openjdk.java.net/%7Eewang/JDK-8031179/webrev.01/>

Eric
On 2014/1/25 8:53, Stuart Marks wrote:
> Hi Eric,
>
> OK, overall this looks good. There are a few adjustments I'd like you 
> to make before I push it for you. Part of this is to get you to do a 
> more complete job of preparing changesets, and part of it is to make 
> my job as a sponsor easier. :-) Oh, and there a couple style issues as 
> well.
>
> 1. In the LookupIPv6 test, REGISTRY_PORT is capitalized, making it 
> appear to be a constant, when in fact it's a variable that contains 
> the result of calling getUnusedRandomPort(). It's also unclear why it 
> needs to be a static. Just make it a local variable with a typical 
> variable name, e.g.
>
>     int port = TestLibrary.getUnusedRandomPort();
>
> and of course make corresponding name changes where it's used.
>
> 2. The creation of the URL string for Naming.lookup() concatenates two 
> string literals "]" + ":". It's probably better to combine these into 
> a single literal "]:". Actually I think it would be preferable to 
> create this URL string using String.format(), so please do this instead.
>
> 3. Prepare a complete changeset that's committed locally to your 
> repository before running webrev. This changeset should include 
> properly formatted commit comments, including the bugid line and 
> Reviewed-by line, and optionally a Summary line. Recent versions of 
> webrev will export a mercurial changeset and include it in the webrev 
> output, instead of just diffs. Using the changeset I can just import 
> and push it, instead of having to edit the changeset myself manually.
>
> 4. When you post the webrev, you should post updated webrevs under a 
> directory with an incremented sequence number (in this case, 
> webrev.01). That way, links to, and comments on, previous webrevs will 
> not be invalidated.
>
> Thanks,
>
> s'marks
>
> On 1/24/14 1:43 AM, Eric Wang wrote:
>> 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
>>>>>>
>>>>>
>>>>
>>>
>>