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

Sat, 25 Jan 2014 01:27:03 -0800 

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

































					Reply via email to