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

Sat Jan 25 00:53:48 UTC 2014

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