Question about the bug https://bugs.openjdk.java.net/browse/JDK-8031179
stuart.marks at oracle.com
Sat Jan 25 00:53:48 UTC 2014
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
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.
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?
> 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:
>> 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
>> 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:
>>> 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?
>>> 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.
>>>> Also need your help to confirm that the rest 9 tests in previous mail don't
>>>> need the /othervm option indeed.
>>>> 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:
>>>>> Tests don't need the /othervm option:
>>>>> 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.
More information about the core-libs-dev