from:"Hamlin Li"

Thanks Roger, sorry for that I missed your response. (most of time I 
focus on mail send to/cc me :) )


On 2019/10/15 10:29 PM, Roger Riggs wrote:

Hi,

1. Replace the creation of the Registry with 
TestLibrary.crateRegistryOnEphemeralPort

and call TestLibrary.getRegistryPort to get the port number.

The intent of the test is still satisified without the timing/port 
allocation issues.


For this test, it failed only after unexportObject is called and port is 
not freed in time or occupied again by others.


So it would be fine to use crateRegistryOnEphemeralPort but will not 
help to resolve this issue.




2. Another approach to testing if the port is in use would be to
assume its still active and do a registry.lookup("xxx").

The response be NotBoundException if the registry is still alive
or will be a RemoteException indicating the port is closed.

If some other application has grabbed the port, some other exception 
RemoteException cause will be thrown.


I'm not sure if RemoteException equals to the port has been closed by 
the registry.


Based on API doc, "|RemoteException 
|- 
if remote communication with the registry failed; if exception is 
a|ServerException|containing an|AccessException|, then the registry 
denies the caller access to perform this operation", in my 
understanding, a) "port is closed" and b) "others grab the port again" 
will cause RemoteException, but RemoteException does not mean it's in 
just one of the 2 situations a) and b).


Thank you

-Hamlin

Re: RFR of JDK-8134599: TEST_BUG: java/rmi/transport/closeServerSocket/CloseServerSocket.java fails intermittently with Address already in use




On 2019/10/16 9:26 AM, Weijun Wang wrote:


Even after this bug is resolved, I'd suggest add some logging and rerun this 
test unchanged multiple times to see what really happened.

Unfortunately, most of time this kind of issue is not easy to reproduce by 
running it multiple times. In my point of view, current logging is sufficient 
for diagnosing.

I see some logging calls inside source code. Maybe you can set a logging config 
file to let it show more?

I'm not sure I get your point, could you help to clarify?

For example, in src/java.rmi/share/classes/sun/rmi/transport/ObjectTable.java, 
there is

if (DGCImpl.dgcLog.isLoggable(Log.VERBOSE)) {
 DGCImpl.dgcLog.log(Log.VERBOSE, "remove object " + oe);
}

So it looks it supports logging. Maybe you can provide a logging config file 
with -Djava.util.logging.config.file and see more info.


Good point, I think we can do further: add logging for not just this 
tests, but for other frequently failed rmi tests.


But as this is failing so frequently we need a quick fix or have to 
problemlist it first, so I just created a bug to track this enhancement: 
https://bugs.openjdk.java.net/browse/JDK-8232352, how do you think about it?


Thank you

-Hamlin



Thanks,
Max


Thank you

-Hamlin


--Max


Thank you

-Hamlin


--Max



On Oct 15, 2019, at 11:04 AM, Hamlin Li  wrote:

Thanks for reviewing, I updated change at: 
http://cr.openjdk.java.net/~mli/8134599/webrev.01/

it does not increase minimum time time and consider timeout factor at the same 
time.

Thank you

-Hamlin

Re: RFR of JDK-8134599: TEST_BUG: java/rmi/transport/closeServerSocket/CloseServerSocket.java fails intermittently with Address already in use




On 2019/10/15 7:50 PM, Weijun Wang wrote:



On Oct 15, 2019, at 5:44 PM, Hamlin Li  wrote:


On 2019/10/15 2:44 PM, Weijun Wang wrote:

I am OK with the change that makes this test more robust.

Thanks Max.

However, I am not an expert in RMI and don't know why the port cannot be 
released. If verifyPortFree(PORT) on line 60 can always succeed right after 
TestLibrary.getUnusedRandomPort() tries to create a ServerSocket at PORT and 
close it, this means the OS underneath spends no time in freeing the port.

No, the port is not freed here, it's only freed by unexportObject.

I meant the auto close inside getUnusedRandomPort().


I think you misunderstood the 2 operations: socket.close and unexportObject.

As I mentioned below, unexportObject is an async operation, here "async" 
is in the RMI level: unexportObject => async close => socket.close => closed





But the original code is a little bit faulty at verifyPortInUse, it should be 
as below, I have also updated the webrev in place: 
http://cr.openjdk.java.net/~mli/8134599/webrev.01/

@@ -101,6 +112,7 @@
  private static void verifyPortInUse(int port) throws IOException {
  try {
  verifyPortFree(port);
+throw new RuntimeException("port is not in use: " + port);


Is UnicastRemoteObject.unexportObject() also doing something similar? I see 
that the ServerSocket inside is closed on TCPTransport.java:280. Is it reliably 
called?

In my understanding, it's not a sync operation, it's async, that means when 
unexportObject returns, it just triggers the port release, does not mean it 
already released the port.

Ah


Even after this bug is resolved, I'd suggest add some logging and rerun this 
test unchanged multiple times to see what really happened.

Unfortunately, most of time this kind of issue is not easy to reproduce by 
running it multiple times. In my point of view, current logging is sufficient 
for diagnosing.

I see some logging calls inside source code. Maybe you can set a logging config 
file to let it show more?


I'm not sure I get your point, could you help to clarify?

Thank you

-Hamlin



--Max


Thank you

-Hamlin


--Max



On Oct 15, 2019, at 11:04 AM, Hamlin Li  wrote:

Thanks for reviewing, I updated change at: 
http://cr.openjdk.java.net/~mli/8134599/webrev.01/

it does not increase minimum time time and consider timeout factor at the same 
time.

Thank you

-Hamlin

Re: RFR of JDK-8134599: Improve the code coverage for ThreadLocal


Thanks Alan, I will add the @modules.

Please let me know your further comments, I haven't push the code yet.

Thank you

-Hamlin

On 2019/10/15 9:46 PM, Alan Bateman wrote:

On 15/10/2019 03:03, Hamlin Li wrote:

Could some help to review it?
I haven't studied the test but one thing that seems to be missing is 
@modules java.base/java.lang:+open to allow the test access TL 
internals. This will be needed once java.base is fully encapsulated.


-Alan

Re: RFR of JDK-8134599: TEST_BUG: java/rmi/transport/closeServerSocket/CloseServerSocket.java fails intermittently with Address already in use




On 2019/10/15 2:44 PM, Weijun Wang wrote:

I am OK with the change that makes this test more robust.

Thanks Max.


However, I am not an expert in RMI and don't know why the port cannot be 
released. If verifyPortFree(PORT) on line 60 can always succeed right after 
TestLibrary.getUnusedRandomPort() tries to create a ServerSocket at PORT and 
close it, this means the OS underneath spends no time in freeing the port.


No, the port is not freed here, it's only freed by unexportObject.

But the original code is a little bit faulty at verifyPortInUse, it 
should be as below, I have also updated the webrev in place: 
http://cr.openjdk.java.net/~mli/8134599/webrev.01/


@@ -101,6 +112,7 @@
 private static void verifyPortInUse(int port) throws IOException {
 try {
 verifyPortFree(port);
+    throw new RuntimeException("port is not in use: " + port);


Is UnicastRemoteObject.unexportObject() also doing something similar? I see 
that the ServerSocket inside is closed on TCPTransport.java:280. Is it reliably 
called?


In my understanding, it's not a sync operation, it's async, that means 
when unexportObject returns, it just triggers the port release, does not 
mean it already released the port.




Even after this bug is resolved, I'd suggest add some logging and rerun this 
test unchanged multiple times to see what really happened.


Unfortunately, most of time this kind of issue is not easy to reproduce 
by running it multiple times. In my point of view, current logging is 
sufficient for diagnosing.


Thank you

-Hamlin



--Max



On Oct 15, 2019, at 11:04 AM, Hamlin Li  wrote:

Thanks for reviewing, I updated change at: 
http://cr.openjdk.java.net/~mli/8134599/webrev.01/

it does not increase minimum time time and consider timeout factor at the same 
time.

Thank you

-Hamlin

Re: RFR of JDK-8209824: Improve the code coverage for ThreadLocal

Thank you for reviewing David. I will update/push the change as you 
suggested.


BTW, sorry for the wrong bug number in email subject, it should be 8209824.

Thank you

-Hamlin

On 2019/10/15 10:29 AM, David Holmes wrote:

Hi Hamlin,

On 15/10/2019 12:03 pm, Hamlin Li wrote:

Could some help to review it?


Basic test seems okay but a few minor suggestions:

1. Please add a public summary to the bug report to convey the same 
information as:


27  * @summary per latest JDK code coverage report, 2 methods 
replaceStaleEntry
  28  *  and prevIndex in ThreadLocal.ThreadLocalMap is not 
touched
  29  *  by any JDK regression tests, this is to trigger the 
code path.


2. In that comment s/is not/are not/

3. Please add a brief comment to the code explaining how the test 
exercises the previously untested methods.


No need to see an updated webrev for the above.

Thanks,
David


Thank you

-Hamlin

On 2019/9/4 3:16 PM, Hamlin Li wrote:

Would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8209824

webrev: http://cr.openjdk.java.net/~mli/8209824/webrev.00/


Thank you

-Hamlin

Re: RFR of JDK-8134599: TEST_BUG: java/rmi/transport/closeServerSocket/CloseServerSocket.java fails intermittently with Address already in use

Thanks for reviewing, I updated change at: 
http://cr.openjdk.java.net/~mli/8134599/webrev.01/


it does not increase minimum time time and consider timeout factor at 
the same time.


Thank you

-Hamlin

On 2019/10/15 10:43 AM, Joe Darcy wrote:

Hello,

Structurally, I'd prefer an approach that doesn't increase the minimum 
time needed to run the test.


Any timeout-like value used within the test code should be sensitive 
to the jtreg timeout factor, as done in the webrev.


Thanks,

-Joe

On 10/14/2019 7:20 PM, Weijun Wang wrote:

+SeanC

The wait might unnecessarily increase the test time. Maybe you can do 
something like this:


    int timeout = 10;
    while (timeout > 0) {
   sleep(one second);
   verifyPortFree && return;
   timeout--;
    }
    throw new Exception(still not freed);

And Sean, back in JDK-8016728 you said "Let's extend it to 1000ms and 
see how test behaves". So what do you think?


Thanks,
Max


On Oct 15, 2019, at 10:03 AM, Hamlin Li  wrote:

Hi,

The test is failing more frequently, could some help to review it?

Thank you

-Hamlin

On 2019/9/4 11:11 AM, Hamlin Li wrote:
some background & comment: in most of failures, the 
"test.timeout.factor" is 10.0 or 8.0, so in the test code this 
factor should be considered in rmi operations such unexporting an 
object.


Thank you

-Hamlin

On 2019/9/4 11:01 AM, Hamlin Li wrote:

Hi,

Would you please review the following patch?

Bug: https://bugs.openjdk.java.net/browse/JDK-8134599

webrev: http://cr.openjdk.java.net/~mli/8134599/webrev.00/


Thank you

-Hamlin

Re: RFR of JDK-8134599: Improve the code coverage for ThreadLocal


Could some help to review it?

Thank you

-Hamlin

On 2019/9/4 3:16 PM, Hamlin Li wrote:

Would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8209824

webrev: http://cr.openjdk.java.net/~mli/8209824/webrev.00/


Thank you

-Hamlin

Re: RFR of JDK-8134599: TEST_BUG: java/rmi/transport/closeServerSocket/CloseServerSocket.java fails intermittently with Address already in use


Hi,

The test is failing more frequently, could some help to review it?

Thank you

-Hamlin

On 2019/9/4 11:11 AM, Hamlin Li wrote:
some background & comment: in most of failures, the 
"test.timeout.factor" is 10.0 or 8.0, so in the test code this factor 
should be considered in rmi operations such unexporting an object.


Thank you

-Hamlin

On 2019/9/4 11:01 AM, Hamlin Li wrote:

Hi,

Would you please review the following patch?

Bug: https://bugs.openjdk.java.net/browse/JDK-8134599

webrev: http://cr.openjdk.java.net/~mli/8134599/webrev.00/


Thank you

-Hamlin

RFR of JDK-8134599: Improve the code coverage for ThreadLocal

2019-09-04 Thread Hamlin Li


Would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8209824

webrev: http://cr.openjdk.java.net/~mli/8209824/webrev.00/


Thank you

-Hamlin

Re: RFR of JDK-8134599: TEST_BUG: java/rmi/transport/closeServerSocket/CloseServerSocket.java fails intermittently with Address already in use

2019-09-03 Thread Hamlin Li

some background & comment: in most of failures, the 
"test.timeout.factor" is 10.0 or 8.0, so in the test code this factor 
should be considered in rmi operations such unexporting an object.


Thank you

-Hamlin

On 2019/9/4 11:01 AM, Hamlin Li wrote:

Hi,

Would you please review the following patch?

Bug: https://bugs.openjdk.java.net/browse/JDK-8134599

webrev: http://cr.openjdk.java.net/~mli/8134599/webrev.00/


Thank you

-Hamlin

RFR of JDK-8134599: TEST_BUG: java/rmi/transport/closeServerSocket/CloseServerSocket.java fails intermittently with Address already in use

2019-09-03 Thread Hamlin Li


Hi,

Would you please review the following patch?

Bug: https://bugs.openjdk.java.net/browse/JDK-8134599

webrev: http://cr.openjdk.java.net/~mli/8134599/webrev.00/


Thank you

-Hamlin

Re: RFR of JDK-8214431: tests failed because can not find removed library folder at test/jdk/lib/testlibrary/jdk/testlibrary


Hi,

Thank you for reviewing this, it's just pushed, I'm sorry for the 
inconvenience.


Thank you

-Hamlin

On 2018/11/29 5:12 AM, Chris Hegarty wrote:

I think this good. Thanks.

-Chris.


On 28 Nov 2018, at 20:32, David Holmes  wrote:

Hi Hamlin,

On 28/11/2018 10:52 pm, Hamlin Li wrote:

Hi David,
Yes, they'd better be removed too, so I create another bug 
https://bugs.openjdk.java.net/browse/JDK-8214435 to track it.

Ok.


With only /lib/testlibrary/, tests will not fail, tests only fail when there is something 
like "run build jdk.testlibrary.*"

Ok.


So, I think in this bug it's OK to just address failed tests, and address 
complete removal of /lib/testlibrary/ in JDK-8214435.

Ok. :)

I would have expected core-libs folk to have reviewed this by now so that it 
could have been pushed! This is causing major disruption to the CI testing! :(

Thanks,
David


Thank you
-Hamlin
On 2018/11/28 8:08 PM, David Holmes wrote:

Hi Hamlin,

On 28/11/2018 9:15 pm, Hamlin Li wrote:

Hi David,

Thank a lot for double checking the usage of testlibrary.

I have updated the patch, http://cr.openjdk.java.net/~mli/8214431/webrev.00/

I'm not sure about the removal of /lib/testlibrary/ from

  @library /lib/testlibrary/ /test/lib

as there are dozens of tests that refer to

  @library /lib/testlibrary/

either directly or via a relative path. Do they all need to be changed ?? Or 
none?

David

PS. I'm finished for the night.


Thank you

-Hamlin

On 2018/11/28 6:24 PM, David Holmes wrote:

Hi Hamlin,

I see a lot more tests that look like they may be affected:

./jdk/com/sun/tools/attach/TempDirTest.java: * @run build jdk.testlibrary.* 
Application RunnerUtil
./jdk/com/sun/tools/attach/PermissionTest.java: * @run build jdk.testlibrary.* 
Application
./jdk/com/sun/tools/attach/BasicTests.java: * @run build jdk.testlibrary.* 
Agent BadAgent RedefineAgent Application RedefineDummy RunnerUtil
./jdk/com/sun/tools/attach/ProviderTest.java: * @run build jdk.testlibrary.* 
SimpleProvider
./jdk/com/sun/jdi/ProcessAttachTest.java: * @build jdk.testlibrary.* 
ProcessAttachTest
./jdk/java/lang/Thread/ThreadStateTest.java: * @build jdk.testlibrary.*
./jdk/java/lang/management/MemoryMXBean/CollectionUsageThreshold.java: * @build 
jdk.testlibrary.* CollectionUsageThreshold MemoryUtil RunUtil
./jdk/java/lang/management/MemoryMXBean/ResetPeakMemoryUsage.java: * @build 
jdk.testlibrary.* ResetPeakMemoryUsage MemoryUtil RunUtil
./jdk/java/lang/management/ThreadMXBean/ThreadMXBeanStateTest.java: * @build 
jdk.testlibrary.*
./jdk/sun/management/jmxremote/startstop/JMXStatusTest.java: * @build 
jdk.testlibrary.* PortAllocator TestApp ManagementAgentJcmd
./jdk/sun/management/jmxremote/startstop/JMXStatusPerfCountersTest.java: * 
@build jdk.testlibrary.* PortAllocator TestApp ManagementAgentJcmd
./jdk/sun/management/jmxremote/bootstrap/SSLConfigFilePermissionTest.java: * 
@build jdk.testlibrary.* jdk.test.lib.Platform Dummy AbstractFilePermissionTest
./jdk/sun/management/jmxremote/bootstrap/PasswordFilePermissionTest.java: * 
@build jdk.testlibrary.* jdk.test.lib.Platform AbstractFilePermissionTest Dummy

Cheers,
David
-

On 28/11/2018 8:14 pm, Hamlin Li wrote:

Would you please review the following patch?

This is a regression by JDK-8211975.

bug: https://bugs.openjdk.java.net/browse/JDK-8214431

patch at the bottom.

Thank you

-Hamlin



diff -r 70adb0f573a7 test/jdk/com/sun/jdi/ProcessAttachTest.java
--- a/test/jdk/com/sun/jdi/ProcessAttachTest.javaWed Nov 28 15:34:43 2018 
+0800
+++ b/test/jdk/com/sun/jdi/ProcessAttachTest.javaWed Nov 28 18:13:49 2018 
+0800
@@ -38,11 +38,10 @@
* @bug 4527279
* @summary Unit test for ProcessAttachingConnector
*
- * @library /lib/testlibrary
* @library /test/lib
* @modules java.management
*  jdk.jdi
- * @build jdk.testlibrary.* ProcessAttachTest
+ * @build ProcessAttachTest
* @run driver ProcessAttachTest
*/

diff -r 70adb0f573a7 test/jdk/java/lang/Thread/ThreadStateTest.java
--- a/test/jdk/java/lang/Thread/ThreadStateTest.javaWed Nov 28 15:34:43 
2018 +0800
+++ b/test/jdk/java/lang/Thread/ThreadStateTest.javaWed Nov 28 18:13:49 
2018 +0800
@@ -30,9 +30,7 @@
*  Thread.getState().
*
* @author  Mandy Chung
- * @library /lib/testlibrary
* @library /test/lib
- * @build jdk.testlibrary.*
* @build jdk.test.lib.LockFreeLogger
* @build ThreadStateTest ThreadStateController
* @run main/othervm -Xmixed ThreadStateTest

Re: RFR of JDK-8214431: tests failed because can not find removed library folder at test/jdk/lib/testlibrary/jdk/testlibrary

I think you mean bootstrap/PasswordFilePermissionTest.java 
bootstrap/SSLConfigFilePermissionTest.java 
startstop/JMXStatusPerfCountersTest.java startstop/JMXStatusTest.java.


They have already been included in 
http://cr.openjdk.java.net/~mli/8214431/webrev.00/


Thank you

-Hamlin

On 2018/11/28 9:09 PM, li.ji...@oracle.com wrote:

Hi Hamlin,

You can find some tests under sun/management/jmxremote/ failed on this 
issue too.


Thanks,
Leo

On 11/28/18 7:15 PM, Hamlin Li wrote:

Hi David,

Thank a lot for double checking the usage of testlibrary.

I have updated the patch, 
http://cr.openjdk.java.net/~mli/8214431/webrev.00/


Thank you

-Hamlin

On 2018/11/28 6:24 PM, David Holmes wrote:

Hi Hamlin,

I see a lot more tests that look like they may be affected:

./jdk/com/sun/tools/attach/TempDirTest.java: * @run build 
jdk.testlibrary.* Application RunnerUtil
./jdk/com/sun/tools/attach/PermissionTest.java: * @run build 
jdk.testlibrary.* Application
./jdk/com/sun/tools/attach/BasicTests.java: * @run build 
jdk.testlibrary.* Agent BadAgent RedefineAgent Application 
RedefineDummy RunnerUtil
./jdk/com/sun/tools/attach/ProviderTest.java: * @run build 
jdk.testlibrary.* SimpleProvider
./jdk/com/sun/jdi/ProcessAttachTest.java: * @build jdk.testlibrary.* 
ProcessAttachTest

./jdk/java/lang/Thread/ThreadStateTest.java: * @build jdk.testlibrary.*
./jdk/java/lang/management/MemoryMXBean/CollectionUsageThreshold.java: 
* @build jdk.testlibrary.* CollectionUsageThreshold MemoryUtil RunUtil
./jdk/java/lang/management/MemoryMXBean/ResetPeakMemoryUsage.java: * 
@build jdk.testlibrary.* ResetPeakMemoryUsage MemoryUtil RunUtil
./jdk/java/lang/management/ThreadMXBean/ThreadMXBeanStateTest.java: 
* @build jdk.testlibrary.*
./jdk/sun/management/jmxremote/startstop/JMXStatusTest.java: * 
@build jdk.testlibrary.* PortAllocator TestApp ManagementAgentJcmd
./jdk/sun/management/jmxremote/startstop/JMXStatusPerfCountersTest.java: 
* @build jdk.testlibrary.* PortAllocator TestApp ManagementAgentJcmd
./jdk/sun/management/jmxremote/bootstrap/SSLConfigFilePermissionTest.java: 
* @build jdk.testlibrary.* jdk.test.lib.Platform Dummy 
AbstractFilePermissionTest
./jdk/sun/management/jmxremote/bootstrap/PasswordFilePermissionTest.java: 
* @build jdk.testlibrary.* jdk.test.lib.Platform 
AbstractFilePermissionTest Dummy


Cheers,
David
-

On 28/11/2018 8:14 pm, Hamlin Li wrote:

Would you please review the following patch?

This is a regression by JDK-8211975.

bug: https://bugs.openjdk.java.net/browse/JDK-8214431

patch at the bottom.

Thank you

-Hamlin

 



diff -r 70adb0f573a7 test/jdk/com/sun/jdi/ProcessAttachTest.java
--- a/test/jdk/com/sun/jdi/ProcessAttachTest.java    Wed Nov 28 
15:34:43 2018 +0800
+++ b/test/jdk/com/sun/jdi/ProcessAttachTest.java    Wed Nov 28 
18:13:49 2018 +0800

@@ -38,11 +38,10 @@
   * @bug 4527279
   * @summary Unit test for ProcessAttachingConnector
   *
- * @library /lib/testlibrary
   * @library /test/lib
   * @modules java.management
   *  jdk.jdi
- * @build jdk.testlibrary.* ProcessAttachTest
+ * @build ProcessAttachTest
   * @run driver ProcessAttachTest
   */

diff -r 70adb0f573a7 test/jdk/java/lang/Thread/ThreadStateTest.java
--- a/test/jdk/java/lang/Thread/ThreadStateTest.java    Wed Nov 28 
15:34:43 2018 +0800
+++ b/test/jdk/java/lang/Thread/ThreadStateTest.java    Wed Nov 28 
18:13:49 2018 +0800

@@ -30,9 +30,7 @@
   *  Thread.getState().
   *
   * @author  Mandy Chung
- * @library /lib/testlibrary
   * @library /test/lib
- * @build jdk.testlibrary.*
   * @build jdk.test.lib.LockFreeLogger
   * @build ThreadStateTest ThreadStateController
   * @run main/othervm -Xmixed ThreadStateTest

Re: RFR of JDK-8214431: tests failed because can not find removed library folder at test/jdk/lib/testlibrary/jdk/testlibrary


Hi David,

Yes, they'd better be removed too, so I create another bug 
https://bugs.openjdk.java.net/browse/JDK-8214435 to track it.


With only /lib/testlibrary/, tests will not fail, tests only fail when 
there is something like "run build jdk.testlibrary.*"


So, I think in this bug it's OK to just address failed tests, and 
address complete removal of /lib/testlibrary/ in JDK-8214435.



Thank you

-Hamlin

On 2018/11/28 8:08 PM, David Holmes wrote:

Hi Hamlin,

On 28/11/2018 9:15 pm, Hamlin Li wrote:

Hi David,

Thank a lot for double checking the usage of testlibrary.

I have updated the patch, 
http://cr.openjdk.java.net/~mli/8214431/webrev.00/


I'm not sure about the removal of /lib/testlibrary/ from

 @library /lib/testlibrary/ /test/lib

as there are dozens of tests that refer to

 @library /lib/testlibrary/

either directly or via a relative path. Do they all need to be changed 
?? Or none?


David

PS. I'm finished for the night.


Thank you

-Hamlin

On 2018/11/28 6:24 PM, David Holmes wrote:

Hi Hamlin,

I see a lot more tests that look like they may be affected:

./jdk/com/sun/tools/attach/TempDirTest.java: * @run build 
jdk.testlibrary.* Application RunnerUtil
./jdk/com/sun/tools/attach/PermissionTest.java: * @run build 
jdk.testlibrary.* Application
./jdk/com/sun/tools/attach/BasicTests.java: * @run build 
jdk.testlibrary.* Agent BadAgent RedefineAgent Application 
RedefineDummy RunnerUtil
./jdk/com/sun/tools/attach/ProviderTest.java: * @run build 
jdk.testlibrary.* SimpleProvider
./jdk/com/sun/jdi/ProcessAttachTest.java: * @build jdk.testlibrary.* 
ProcessAttachTest

./jdk/java/lang/Thread/ThreadStateTest.java: * @build jdk.testlibrary.*
./jdk/java/lang/management/MemoryMXBean/CollectionUsageThreshold.java: 
* @build jdk.testlibrary.* CollectionUsageThreshold MemoryUtil RunUtil
./jdk/java/lang/management/MemoryMXBean/ResetPeakMemoryUsage.java: * 
@build jdk.testlibrary.* ResetPeakMemoryUsage MemoryUtil RunUtil
./jdk/java/lang/management/ThreadMXBean/ThreadMXBeanStateTest.java: 
* @build jdk.testlibrary.*
./jdk/sun/management/jmxremote/startstop/JMXStatusTest.java: * 
@build jdk.testlibrary.* PortAllocator TestApp ManagementAgentJcmd
./jdk/sun/management/jmxremote/startstop/JMXStatusPerfCountersTest.java: 
* @build jdk.testlibrary.* PortAllocator TestApp ManagementAgentJcmd
./jdk/sun/management/jmxremote/bootstrap/SSLConfigFilePermissionTest.java: 
* @build jdk.testlibrary.* jdk.test.lib.Platform Dummy 
AbstractFilePermissionTest
./jdk/sun/management/jmxremote/bootstrap/PasswordFilePermissionTest.java: 
* @build jdk.testlibrary.* jdk.test.lib.Platform 
AbstractFilePermissionTest Dummy


Cheers,
David
-

On 28/11/2018 8:14 pm, Hamlin Li wrote:

Would you please review the following patch?

This is a regression by JDK-8211975.

bug: https://bugs.openjdk.java.net/browse/JDK-8214431

patch at the bottom.

Thank you

-Hamlin

 



diff -r 70adb0f573a7 test/jdk/com/sun/jdi/ProcessAttachTest.java
--- a/test/jdk/com/sun/jdi/ProcessAttachTest.java    Wed Nov 28 
15:34:43 2018 +0800
+++ b/test/jdk/com/sun/jdi/ProcessAttachTest.java    Wed Nov 28 
18:13:49 2018 +0800

@@ -38,11 +38,10 @@
   * @bug 4527279
   * @summary Unit test for ProcessAttachingConnector
   *
- * @library /lib/testlibrary
   * @library /test/lib
   * @modules java.management
   *  jdk.jdi
- * @build jdk.testlibrary.* ProcessAttachTest
+ * @build ProcessAttachTest
   * @run driver ProcessAttachTest
   */

diff -r 70adb0f573a7 test/jdk/java/lang/Thread/ThreadStateTest.java
--- a/test/jdk/java/lang/Thread/ThreadStateTest.java    Wed Nov 28 
15:34:43 2018 +0800
+++ b/test/jdk/java/lang/Thread/ThreadStateTest.java    Wed Nov 28 
18:13:49 2018 +0800

@@ -30,9 +30,7 @@
   *  Thread.getState().
   *
   * @author  Mandy Chung
- * @library /lib/testlibrary
   * @library /test/lib
- * @build jdk.testlibrary.*
   * @build jdk.test.lib.LockFreeLogger
   * @build ThreadStateTest ThreadStateController
   * @run main/othervm -Xmixed ThreadStateTest

Re: RFR of JDK-8214431: tests failed because can not find removed library folder at test/jdk/lib/testlibrary/jdk/testlibrary


Hi David,

Thank a lot for double checking the usage of testlibrary.

I have updated the patch, http://cr.openjdk.java.net/~mli/8214431/webrev.00/

Thank you

-Hamlin

On 2018/11/28 6:24 PM, David Holmes wrote:

Hi Hamlin,

I see a lot more tests that look like they may be affected:

./jdk/com/sun/tools/attach/TempDirTest.java: * @run build 
jdk.testlibrary.* Application RunnerUtil
./jdk/com/sun/tools/attach/PermissionTest.java: * @run build 
jdk.testlibrary.* Application
./jdk/com/sun/tools/attach/BasicTests.java: * @run build 
jdk.testlibrary.* Agent BadAgent RedefineAgent Application 
RedefineDummy RunnerUtil
./jdk/com/sun/tools/attach/ProviderTest.java: * @run build 
jdk.testlibrary.* SimpleProvider
./jdk/com/sun/jdi/ProcessAttachTest.java: * @build jdk.testlibrary.* 
ProcessAttachTest

./jdk/java/lang/Thread/ThreadStateTest.java: * @build jdk.testlibrary.*
./jdk/java/lang/management/MemoryMXBean/CollectionUsageThreshold.java: 
* @build jdk.testlibrary.* CollectionUsageThreshold MemoryUtil RunUtil
./jdk/java/lang/management/MemoryMXBean/ResetPeakMemoryUsage.java: * 
@build jdk.testlibrary.* ResetPeakMemoryUsage MemoryUtil RunUtil
./jdk/java/lang/management/ThreadMXBean/ThreadMXBeanStateTest.java: * 
@build jdk.testlibrary.*
./jdk/sun/management/jmxremote/startstop/JMXStatusTest.java: * @build 
jdk.testlibrary.* PortAllocator TestApp ManagementAgentJcmd
./jdk/sun/management/jmxremote/startstop/JMXStatusPerfCountersTest.java: 
* @build jdk.testlibrary.* PortAllocator TestApp ManagementAgentJcmd
./jdk/sun/management/jmxremote/bootstrap/SSLConfigFilePermissionTest.java: 
* @build jdk.testlibrary.* jdk.test.lib.Platform Dummy 
AbstractFilePermissionTest
./jdk/sun/management/jmxremote/bootstrap/PasswordFilePermissionTest.java: 
* @build jdk.testlibrary.* jdk.test.lib.Platform 
AbstractFilePermissionTest Dummy


Cheers,
David
-

On 28/11/2018 8:14 pm, Hamlin Li wrote:

Would you please review the following patch?

This is a regression by JDK-8211975.

bug: https://bugs.openjdk.java.net/browse/JDK-8214431

patch at the bottom.

Thank you

-Hamlin



diff -r 70adb0f573a7 test/jdk/com/sun/jdi/ProcessAttachTest.java
--- a/test/jdk/com/sun/jdi/ProcessAttachTest.java    Wed Nov 28 
15:34:43 2018 +0800
+++ b/test/jdk/com/sun/jdi/ProcessAttachTest.java    Wed Nov 28 
18:13:49 2018 +0800

@@ -38,11 +38,10 @@
   * @bug 4527279
   * @summary Unit test for ProcessAttachingConnector
   *
- * @library /lib/testlibrary
   * @library /test/lib
   * @modules java.management
   *  jdk.jdi
- * @build jdk.testlibrary.* ProcessAttachTest
+ * @build ProcessAttachTest
   * @run driver ProcessAttachTest
   */

diff -r 70adb0f573a7 test/jdk/java/lang/Thread/ThreadStateTest.java
--- a/test/jdk/java/lang/Thread/ThreadStateTest.java    Wed Nov 28 
15:34:43 2018 +0800
+++ b/test/jdk/java/lang/Thread/ThreadStateTest.java    Wed Nov 28 
18:13:49 2018 +0800

@@ -30,9 +30,7 @@
   *  Thread.getState().
   *
   * @author  Mandy Chung
- * @library /lib/testlibrary
   * @library /test/lib
- * @build jdk.testlibrary.*
   * @build jdk.test.lib.LockFreeLogger
   * @build ThreadStateTest ThreadStateController
   * @run main/othervm -Xmixed ThreadStateTest

RFR of JDK-8214431: tests failed because can not find removed library folder at test/jdk/lib/testlibrary/jdk/testlibrary


Would you please review the following patch?

This is a regression by JDK-8211975.

bug: https://bugs.openjdk.java.net/browse/JDK-8214431

patch at the bottom.

Thank you

-Hamlin



diff -r 70adb0f573a7 test/jdk/com/sun/jdi/ProcessAttachTest.java
--- a/test/jdk/com/sun/jdi/ProcessAttachTest.java    Wed Nov 28 15:34:43 
2018 +0800
+++ b/test/jdk/com/sun/jdi/ProcessAttachTest.java    Wed Nov 28 18:13:49 
2018 +0800

@@ -38,11 +38,10 @@
  * @bug 4527279
  * @summary Unit test for ProcessAttachingConnector
  *
- * @library /lib/testlibrary
  * @library /test/lib
  * @modules java.management
  *  jdk.jdi
- * @build jdk.testlibrary.* ProcessAttachTest
+ * @build ProcessAttachTest
  * @run driver ProcessAttachTest
  */

diff -r 70adb0f573a7 test/jdk/java/lang/Thread/ThreadStateTest.java
--- a/test/jdk/java/lang/Thread/ThreadStateTest.java    Wed Nov 28 
15:34:43 2018 +0800
+++ b/test/jdk/java/lang/Thread/ThreadStateTest.java    Wed Nov 28 
18:13:49 2018 +0800

@@ -30,9 +30,7 @@
  *  Thread.getState().
  *
  * @author  Mandy Chung
- * @library /lib/testlibrary
  * @library /test/lib
- * @build jdk.testlibrary.*
  * @build jdk.test.lib.LockFreeLogger
  * @build ThreadStateTest ThreadStateController
  * @run main/othervm -Xmixed ThreadStateTest

RFR of JDK-8211975: move testlibrary/jdk/testlibrary/OptimalCapacity.java to top-level library

2018-11-27 Thread Hamlin Li


Would you please review the following patch?

https://bugs.openjdk.java.net/browse/JDK-8211975

http://cr.openjdk.java.net/~mli/8211975/webrev.00/


Thank you

-Hamlin

Re: RFR of JDK-8211974,move test/jdk/lib/testlibrary/java/util/jar/*.java to top-level library or a local library

2018-11-19 Thread Hamlin Li


Hi Igor,

I have updated the bugs to avoid review confusion, patch is not changed.

Thank you

-Hamlin

On 2018/11/15 9:36 AM, Igor Ignatyev wrote:

Hi Hamlin,

Although I understand your reasoning, I do share Amy's concerns on doing less 
than the RFEs ask for (as w/ 8211972). so I'd suggest you to split your patch 
into 4 separate patches and RFRs, one per original RFE. this won't just return 
sanity to reviewers and review, reduce chance of leaving already fixed bugs 
forgotten/forsaken, but will also make it possible to push ones which don't 
cause any concerns.

Thanks,
-- Igor


On Oct 11, 2018, at 11:28 PM, Amy Lu  wrote:

On 2018/10/12 2:16 PM, Hamlin Li wrote:

yes, e.g. https://bugs.openjdk.java.net/browse/JDK-8212033

It seems mentioned bug is duplicate with

JDK-8211972: remove testlibrary/java/util/jar/Compiler.java - "suggest removing and 
using jdk.test.lib.compiler.InMemoryJavaCompiler instead"

which is included in this changeset.

Thanks,
Amy


Thank you

-Hamlin


On 2018/10/12 2:13 PM, Amy Lu wrote:

Hi, Hamlin

- test/lib/jdk/test/lib/compiler/Compiler.java (was 
test/jdk/lib/testlibrary/java/util/jar/Compiler.java)
Any future plan to "merge" it with existing jdk.test.lib.compiler.CompilerUtils?

- test/lib/jdk/test/lib/util/JarBuilder.java (was 
test/jdk/lib/testlibrary/java/util/jar/JarBuilder.java)
Any future plan to "merge" it with existing jdk.test.lib.util.JarUtils?

Thanks,
Amy

On 2018/10/12 2:00 PM, Hamlin Li wrote:

Hi Igor,

It's updated in place http://cr.openjdk.java.net/~mli/8211974/webrev.00/, 
please review it again.

Thank you

-Hamlin


On 2018/10/12 1:34 PM, Igor Ignatyev wrote:

Hi Hamlin,

could you please move jdk.test.lib.util.Compiler to j.t.l.compiler package? we 
use this package for classes which have dependency on jdk.compiler and/or 
java.compiler module.

it'd also be nice to put CreateMultiReleaseTestJars into a named package.

-- Igor

On Oct 11, 2018, at 10:23 PM, Hamlin Li  wrote:

would you please review the following patch?

bug:

https://bugs.openjdk.java.net/browse/JDK-8211974

https://bugs.openjdk.java.net/browse/JDK-8211972

https://bugs.openjdk.java.net/browse/JDK-8211973

https://bugs.openjdk.java.net/browse/JDK-8211979

webrev: http://cr.openjdk.java.net/~mli/8211974/webrev.00/

Thank you

-Hamlin

Re: RFR of JDK-8211974,move testlibrary/java/util/jar/CreateMultiReleaseTestJars.java to a separate testlibrary

2018-10-15 Thread Hamlin Li


Ping...


On 2018/10/12 2:00 PM, Hamlin Li wrote:

Hi Igor,

It's updated in place 
http://cr.openjdk.java.net/~mli/8211974/webrev.00/, please review it 
again.


Thank you

-Hamlin


On 2018/10/12 1:34 PM, Igor Ignatyev wrote:

Hi Hamlin,

could you please move jdk.test.lib.util.Compiler to j.t.l.compiler 
package? we use this package for classes which have dependency on 
jdk.compiler and/or java.compiler module.


it'd also be nice to put CreateMultiReleaseTestJars into a named 
package.


-- Igor

On Oct 11, 2018, at 10:23 PM, Hamlin Li  wrote:

would you please review the following patch?

bug:

   https://bugs.openjdk.java.net/browse/JDK-8211974

   https://bugs.openjdk.java.net/browse/JDK-8211972

   https://bugs.openjdk.java.net/browse/JDK-8211973

   https://bugs.openjdk.java.net/browse/JDK-8211979

webrev: http://cr.openjdk.java.net/~mli/8211974/webrev.00/

Thank you

-Hamlin

Re: RFR of JDK-8211974,move testlibrary/java/util/jar/CreateMultiReleaseTestJars.java to a separate testlibrary


yes, e.g. https://bugs.openjdk.java.net/browse/JDK-8212033

Thank you

-Hamlin


On 2018/10/12 2:13 PM, Amy Lu wrote:

Hi, Hamlin

- test/lib/jdk/test/lib/compiler/Compiler.java (was 
test/jdk/lib/testlibrary/java/util/jar/Compiler.java)
Any future plan to "merge" it with existing 
jdk.test.lib.compiler.CompilerUtils?


- test/lib/jdk/test/lib/util/JarBuilder.java (was 
test/jdk/lib/testlibrary/java/util/jar/JarBuilder.java)

Any future plan to "merge" it with existing jdk.test.lib.util.JarUtils?

Thanks,
Amy

On 2018/10/12 2:00 PM, Hamlin Li wrote:

Hi Igor,

It's updated in place 
http://cr.openjdk.java.net/~mli/8211974/webrev.00/, please review it 
again.


Thank you

-Hamlin


On 2018/10/12 1:34 PM, Igor Ignatyev wrote:

Hi Hamlin,

could you please move jdk.test.lib.util.Compiler to j.t.l.compiler 
package? we use this package for classes which have dependency on 
jdk.compiler and/or java.compiler module.


it'd also be nice to put CreateMultiReleaseTestJars into a named 
package.


-- Igor

On Oct 11, 2018, at 10:23 PM, Hamlin Li  wrote:

would you please review the following patch?

bug:

   https://bugs.openjdk.java.net/browse/JDK-8211974

   https://bugs.openjdk.java.net/browse/JDK-8211972

   https://bugs.openjdk.java.net/browse/JDK-8211973

   https://bugs.openjdk.java.net/browse/JDK-8211979

webrev: http://cr.openjdk.java.net/~mli/8211974/webrev.00/

Thank you

-Hamlin

Re: RFR of JDK-8211974,move testlibrary/java/util/jar/CreateMultiReleaseTestJars.java to a separate testlibrary


Hi Igor,

It's updated in place 
http://cr.openjdk.java.net/~mli/8211974/webrev.00/, please review it again.


Thank you

-Hamlin


On 2018/10/12 1:34 PM, Igor Ignatyev wrote:

Hi Hamlin,

could you please move jdk.test.lib.util.Compiler to j.t.l.compiler package? we 
use this package for classes which have dependency on jdk.compiler and/or 
java.compiler module.

it'd also be nice to put CreateMultiReleaseTestJars into a named package.

-- Igor
  

On Oct 11, 2018, at 10:23 PM, Hamlin Li  wrote:

would you please review the following patch?

bug:

   https://bugs.openjdk.java.net/browse/JDK-8211974

   https://bugs.openjdk.java.net/browse/JDK-8211972

   https://bugs.openjdk.java.net/browse/JDK-8211973

   https://bugs.openjdk.java.net/browse/JDK-8211979

webrev: http://cr.openjdk.java.net/~mli/8211974/webrev.00/

Thank you

-Hamlin

RFR of JDK-8211974,move testlibrary/java/util/jar/CreateMultiReleaseTestJars.java to a separate testlibrary


would you please review the following patch?

bug:

  https://bugs.openjdk.java.net/browse/JDK-8211974

  https://bugs.openjdk.java.net/browse/JDK-8211972

  https://bugs.openjdk.java.net/browse/JDK-8211973

  https://bugs.openjdk.java.net/browse/JDK-8211979

webrev: http://cr.openjdk.java.net/~mli/8211974/webrev.00/

Thank you

-Hamlin

Re: RFR of JDK-8186610,move ModuleUtils to top-level testlibrary


Hi Igor,

It's updated in place 
http://cr.openjdk.java.net/~mli/8186610/webrev.01/, please help to 
review again/./


Thank you

-Hamlin


On 2018/10/12 2:51 AM, Igor Ignatyev wrote:

Hi Hamlin,

the key point here, ModuleTargetHelper.java is not a "library" class, 
it's a part of tools/jlink/plugins/SystemModuleDescriptors/ tests.


thanks for updating webrev, I have one nit --- given ModuleUtils is 
the only class in jdk/test/lib/module package, I doubt we need to 
introduce this package, ModuleUtils can be put into j.t.l.util package.


-- Igor.

On Oct 10, 2018, at 11:31 PM, Hamlin Li <mailto:huaming...@oracle.com>> wrote:


Thank you clarifying Igor.

Moving ModuleTargetHelper to local folder has a drawback: it's hard 
for future maintainer to found it if they need to use it in other 
places, that make it an "*invisible*" library class.


Although I don't fully agree with you, I have updated the webrev as 
you suggested: http://cr.openjdk.java.net/~mli/8186610/webrev.01/


Thank you

-Hamlin


On 2018/10/11 11:38 AM, Igor Ignatyev wrote:
b/c this will make test library modularization[1] somewhat 
troublesome, also I ain't sure if ModuleTargetHelper really needs to 
be placed into the top-level library regardless of its dependency on 
non-public API. "promoting" test library class to the top-level 
library comes w/ increased maintenance costs, the parent task[2] 
explains that in more details.


[1] https://bugs.openjdk.java.net/browse/JDK-8211358
[2] https://bugs.openjdk.java.net/browse/JDK-8211290

-- Igor

On Oct 10, 2018, at 8:26 PM, Hamlin Li <mailto:huaming...@oracle.com>> wrote:


Hi Igor,

Would you please clarify your concern further? I mean why 
ModuleTargetHelper can be put to top level when it use non-public 
APIs e.g. jdk.internal.module.*?


Thank you

-Hamlin

On 2018/10/11 11:08 AM, Igor Ignatyev wrote:

Hi Hamlin,

as ModuleTargetHelper uses non-public API, I'd prefer not to have 
in a common test library, and 8211976 suggests moving it closer to 
tests. could you please explain why you decided to put it into the 
library instead?


Thanks,
-- Igor

- Original Message -
From: huaming...@oracle.com <mailto:huaming...@oracle.com>
To: core-libs-dev@openjdk.java.net 
<mailto:core-libs-dev@openjdk.java.net>
Sent: Wednesday, October 10, 2018 7:40:34 PM GMT -08:00 US/Canada 
Pacific

Subject: RFR of JDK-8186610,move ModuleUtils to top-level testlibrary

Would you please review the following patch?

bug:

https://bugs.openjdk.java.net/browse/JDK-8186610

https://bugs.openjdk.java.net/browse/JDK-8211976

webrev: http://cr.openjdk.java.net/~mli/8186610/ 
<http://cr.openjdk.java.net/%7Emli/8186610/>



Thank you

-Hamlin

Re: RFR of JDK-8186610,move ModuleUtils to top-level testlibrary

2018-10-10 Thread Hamlin Li


Thank you clarifying Igor.

Moving ModuleTargetHelper to local folder has a drawback: it's hard for 
future maintainer to found it if they need to use it in other places, 
that make it an "*invisible*" library class.


Although I don't fully agree with you, I have updated the webrev as you 
suggested: http://cr.openjdk.java.net/~mli/8186610/webrev.01/


Thank you

-Hamlin


On 2018/10/11 11:38 AM, Igor Ignatyev wrote:
b/c this will make test library modularization[1] somewhat 
troublesome, also I ain't sure if ModuleTargetHelper really needs to 
be placed into the top-level library regardless of its dependency on 
non-public API. "promoting" test library class to the top-level 
library comes w/ increased maintenance costs, the parent task[2] 
explains that in more details.


[1] https://bugs.openjdk.java.net/browse/JDK-8211358
[2] https://bugs.openjdk.java.net/browse/JDK-8211290

-- Igor

On Oct 10, 2018, at 8:26 PM, Hamlin Li <mailto:huaming...@oracle.com>> wrote:


Hi Igor,

Would you please clarify your concern further? I mean why 
ModuleTargetHelper can be put to top level when it use non-public 
APIs e.g. jdk.internal.module.*?


Thank you

-Hamlin

On 2018/10/11 11:08 AM, Igor Ignatyev wrote:

Hi Hamlin,

as ModuleTargetHelper uses non-public API, I'd prefer not to have in 
a common test library, and 8211976 suggests moving it closer to 
tests. could you please explain why you decided to put it into the 
library instead?


Thanks,
-- Igor

- Original Message -
From: huaming...@oracle.com <mailto:huaming...@oracle.com>
To: core-libs-dev@openjdk.java.net 
<mailto:core-libs-dev@openjdk.java.net>
Sent: Wednesday, October 10, 2018 7:40:34 PM GMT -08:00 US/Canada 
Pacific

Subject: RFR of JDK-8186610,move ModuleUtils to top-level testlibrary

Would you please review the following patch?

bug:

https://bugs.openjdk.java.net/browse/JDK-8186610

https://bugs.openjdk.java.net/browse/JDK-8211976

webrev: http://cr.openjdk.java.net/~mli/8186610/ 
<http://cr.openjdk.java.net/%7Emli/8186610/>



Thank you

-Hamlin

Re: RFR of JDK-8186610,move ModuleUtils to top-level testlibrary

2018-10-10 Thread Hamlin Li


Hi Igor,

Would you please clarify your concern further? I mean why 
ModuleTargetHelper can be put to top level when it use non-public APIs 
e.g. jdk.internal.module.*?


Thank you

-Hamlin

On 2018/10/11 11:08 AM, Igor Ignatyev wrote:

Hi Hamlin,

as ModuleTargetHelper uses non-public API, I'd prefer not to have in a common 
test library, and 8211976 suggests moving it closer to tests. could you please 
explain why you decided to put it into the library instead?

Thanks,
-- Igor

- Original Message -
From: huaming...@oracle.com
To: core-libs-dev@openjdk.java.net
Sent: Wednesday, October 10, 2018 7:40:34 PM GMT -08:00 US/Canada Pacific
Subject: RFR of JDK-8186610,move ModuleUtils to top-level testlibrary

Would you please review the following patch?

bug:

    https://bugs.openjdk.java.net/browse/JDK-8186610

    https://bugs.openjdk.java.net/browse/JDK-8211976

webrev: http://cr.openjdk.java.net/~mli/8186610/


Thank you

-Hamlin

RFR of JDK-8186610,move ModuleUtils to top-level testlibrary

2018-10-10 Thread Hamlin Li


Would you please review the following patch?

bug:

  https://bugs.openjdk.java.net/browse/JDK-8186610

  https://bugs.openjdk.java.net/browse/JDK-8211976

webrev: http://cr.openjdk.java.net/~mli/8186610/


Thank you

-Hamlin

RFR of JDK-8202756,move FilterUSRTest.java to openJDK

2018-05-07 Thread Hamlin Li


Would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8202756

webrev: http://cr.openjdk.java.net/~mli/8202756/webrev.00/


Thank you

-Hamlin

Re: RFR of JDK-8202291,java/rmi/Naming/LookupIPv6.java failed with Connection refused

2018-05-06 Thread Hamlin Li


Is someone available to review the following patch?

Thank you

-Hamlin


On 28/04/2018 11:32 AM, Hamlin Li wrote:

would you please review the patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8202291

webrev: http://cr.openjdk.java.net/~mli/8202291/webrev.00/

In original code, there is a window between get registry and rebind 
which could cause connection refuse.



Thank you

-Hamlin

Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file

2018-04-28 Thread Hamlin Li


Hi Joe,

just a minor comment about the test in ReadWriteString.java.

For both testMalformedWrite and testMalformedRead, temp files are not 
deleted, as Files.deleteIfExists(path); is skipped by the expected 
IOException. File.deleteOnExit() may help.


Thank you

-Hamlin


On 27/04/2018 12:50 PM, Joe Wang wrote:

Hi,

We're looking into adding methods to Files to read a file into a 
String/write a String to a file. Below is the current proposal. Please 
review.


JBS: https://bugs.openjdk.java.net/browse/JDK-8201276

webrev: http://cr.openjdk.java.net/~joehw/jdk11/8201276/webrev/

specdiff: 
http://cr.openjdk.java.net/~joehw/jdk11/8201276/specdiff/java/nio/file/Files.html



Thanks,
Joe

RFR of JDK-8202291,java/rmi/Naming/LookupIPv6.java failed with Connection refused

2018-04-27 Thread Hamlin Li


would you please review the patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8202291

webrev: http://cr.openjdk.java.net/~mli/8202291/webrev.00/

In original code, there is a window between get registry and rebind 
which could cause connection refuse.



Thank you

-Hamlin

Re: RFR of JDK-8201469,test under java/rmi should be restricted to not run concurrently

2018-04-25 Thread Hamlin Li


Hi Paul, Joe,

Thank you for the comments.

Following directories are added in the exclusiveAccess.dirs list:

*/  java/rmi   (so, java/rmi/Naming is removed)/**/
/**/  com/sun/jndi/rmi/**/
/**/  sun/rmi/*

for the test/bug number and time cost please check following table, here 
time cost is the number on my local machine.


added directories 	test number 	run time 	subdirectories 	test number 
bug number

java/rmi120 

run time in -conc:4, 8:09 (14:15:16 - 14:07:07)

run time in sequence, 18:17 (14:44:00 - 14:25:43)


MarshalledObject

4

0
Naming  6   2
RMISecurityManager/checkPackageAccess   1   0
RemoteException/chaining1   0
activation  31  23
dgc 4   0
invalidName 1   0
module  1   0
registry9   1
reliability 3   2
server  45  7
transport   13  6
com/sun/jndi/rmi2   


0
sun/rmi 17  

run time in -conc:4, 00:44 (14:52:08 - 14:51:24)

run time in sequence, 00:51 (14:48:10 - 14:47:19)



2


@Paul,

Good suggestion, I tried to figure out a subset, but most of 
intermittent rmi test failures happen in java/rmi/[activation, server, 
transport, Naming, reliability, registry] and sun/rmi, they are almost 
all of rmi tests. I'm not sure if it's worth to subset them. I can do it 
if you think it's necessary.


( [1] lists rmi open issue in open area. )


Thank you

-Hamlin


[1] 
https://bugs.openjdk.java.net/issues/?jql=project%20%3D%20jdk%20and%20component%20%3D%20core-libs%20and%20Subcomponent%20%3D%20java.rmi%20and%20status%20in%20(open%2C%20new%2C%20%22In%20Progress%22)%20ORDER%20BY%20updatedDate%20DESC



On 25/04/2018 12:14 PM, joe darcy wrote:
Combining the reformatting and test additions makes the patch more 
difficult to review. The net effect seems to be adding the following 
directories to the exclusiveAccess.dirs list:


  44 sun/management/jmxremote \
  45 sun/rmi \
  46 sun/security/mscapi \
  47 sun/tools/jstatd

Is that correct?

Please characterize the number of tests added to the list and the 
expected performance impact on the tier 3 test group.


Thanks,

-Joe


On 4/23/2018 11:47 AM, Paul Sandoz wrote:

Hi Hamlin,

Do you have a sense of what rmi tests are problematic to run 
concurrently? Maybe you can subset to reduce the increased impact on 
execution time?


Paul.


On Apr 20, 2018, at 12:21 AM, Hamlin Li  wrote:

Is someone available to review the following patch?

Thank you

-Hamlin


On 19/04/2018 4:17 PM, Hamlin Li wrote:

would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8201469

http://cr.openjdk.java.net/~mli/8201469/webrev.00/

( For othervm.dirs property, I just reformat it. )


In my test result, with jtreg option "-concurrency:4", after apply 
the patch, tier3 tests on different platforms will be slower about 
1.5~2.0 times than before.
I think stability of tests are more important than executing time, 
how do you think about it?


Thank you
-Hamlin

Re: RFR of JDK-8201469,test under java/rmi should be restricted to not run concurrently

2018-04-20 Thread Hamlin Li


Is someone available to review the following patch?

Thank you

-Hamlin


On 19/04/2018 4:17 PM, Hamlin Li wrote:

would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8201469

http://cr.openjdk.java.net/~mli/8201469/webrev.00/

( For othervm.dirs property, I just reformat it. )


In my test result, with jtreg option "-concurrency:4", after apply the 
patch, tier3 tests on different platforms will be slower about 1.5~2.0 
times than before.
I think stability of tests are more important than executing time, how 
do you think about it?


Thank you
-Hamlin

RFR of JDK-8201469,test under java/rmi should be restricted to not run concurrently

2018-04-19 Thread Hamlin Li


would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8201469

http://cr.openjdk.java.net/~mli/8201469/webrev.00/

( For othervm.dirs property, I just reformat it. )


In my test result, with jtreg option "-concurrency:4", after apply the 
patch, tier3 tests on different platforms will be slower about 1.5~2.0 
times than before.
I think stability of tests are more important than executing time, how 
do you think about it?


Thank you
-Hamlin

Re: RFR of JDK-8078221, TEST_BUG: java/rmi/Naming/DefaultRegistryPort.java fails intermittently

2018-04-12 Thread Hamlin Li


Hi Roger,

Thank you for reviewing, will push the code after improve the exception 
message.


-Hamlin


On 13/04/2018 3:41 AM, Roger Riggs wrote:

Hi Hamlin,

Looks ok.

Please improve the exception message at line 76: to make it clear that 
the retries were

exhausted and not some other cause or exception.

Thanks, Roger


On 4/11/2018 10:47 PM, Hamlin Li wrote:

would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8078221

webrev: http://cr.openjdk.java.net/~mli/8078221/webrev.00/


Thank you

-Hamlin

RFR of JDK-8078221, TEST_BUG: java/rmi/Naming/DefaultRegistryPort.java fails intermittently

2018-04-11 Thread Hamlin Li


would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8078221

webrev: http://cr.openjdk.java.net/~mli/8078221/webrev.00/


Thank you

-Hamlin

Re: RFR of JDK-8188897: java/rmi/registry/reexport/Reexport.java failed with Port already in use

2018-04-09 Thread Hamlin Li



On 10/04/2018 3:09 AM, Roger Riggs wrote:

Hi Hamlin,

Hi Roger,

Thank you for review.


Great, the new busy port algorithm looks better.
The port assigned will always be one that was available and is now 
busy to prevent the registry creation.


As expected, there is a small window  between the (try-with-finally) 
close of the server socket channel
and the 2nd createReg.  But that can't be avoided.  If the port is 
re-used in that gap
the test will fail.  (And the exception handler at 77 will see an 
in-use and retry).

Yes, you're right.


67: The exception being caught is already one thrown by 
TestLibrary.bomb so it would be
cleaner to just re-throw e;  or better yet, just don't bother to catch 
the exception.

That exception should cause the test to fail.
It's to catch IOException by ServerSocketChannel.open, bind. Seems it's 
a little confusing, so I declare at main function to throw IOException.


It may be personal coding style but the createReg method with a 
boolean flag to suppress
the exception is just more confusing that putting the code in-line in 
the two places it is used.
I think it's mainly because of original parameter name remoteOK, so 
rename it as expectException, and move it to the last parameter.


new webrev is updated in place:
http://cr.openjdk.java.net/~mli/8188897/webrev.02/

Thank you
-Hamlin


Thanks, Roger


On 4/8/2018 4:10 AM, Hamlin Li wrote:

Hi Roger,

I have changed to not use RegistryVM at all, please review the patch: 
http://cr.openjdk.java.net/~mli/8188897/webrev.02/


Thank you

-Hamlin


On 04/04/2018 10:15 PM, Roger Riggs wrote:

Hi Hamlin,

Reexport.java:

I think this change to use a separate process for the 2nd registry 
changes the test so that it does not
address the original test case.  The original problem was the 
incorrect retention of an object in
the object table when the create of a registry in the same process 
failed.
Finally being able to create the registry in the same process 
assured that the object was not

retained in the object table.

Go back to creating the 2nd registry in the test process.


RegistryVM.java: 2, the copyright update should be  "2017, 2018," 
range.


(I'm  really not a fan of all the RegistryVM methods with the same 
name and minimal and implicit
differences in their functions.  When reading the test, you have to 
go and read the RegistryVM method
to see what it does.  I would have preferred that the full 
createRegistryVM (out, err, options, port) method
was used directly in the tests.  In the case of a method used once, 
it is an inconvenience method, not a convenience).


The new terminate() method is quite similar to the existing 
cleanup() method which does not wait.
It would be a good cleanup to figure out if another method is really 
needed.
The override is going to change the behavior of the existing uses of 
terminate().

It should be checked that it does not break any existing uses.

178: 187: The method comments are not consistent, one says forcibly 
and the other gracefully

but both call requestExit and both call destroy().

Thanks, Roger


On 4/3/2018 11:35 PM, Hamlin Li wrote:

Hi Joe, Roger,

Thank you for reviewing, I have refactored the test more and fix as 
you suggested.


please review http://cr.openjdk.java.net/~mli/8188897/webrev.01/

Thank you

-Hamlin


On 04/04/2018 10:42 AM, Joseph D. Darcy wrote:

Hello,

Some general comments on the coding for tests like this:

* It is preferable to avoid sleep in tests to avoid increasing the 
minimum amount of time a test takes to run. This helps limit the 
overall time it takes the test suite to run.


* If timeouts are used, it is recommend to factor the maximum time 
waited with the jtreg timeout scaling factor; I don't recall its 
exact name. In other words, many of our tests are run on heavily 
loaded systems and the tests take longer than run than on typical 
laptops and workstations so jtreg is invoked with an timeout 
scaling factor. Individual tests should be sensitive to this 
scaling factor for any internal timeout that need to be used.


HTH,

-Joe

On 4/3/2018 7:48 AM, Roger Riggs wrote:

Hi Hamlin,

Instead of a simple time delay, it would be useful to wait for 
the RegistryVM to terminate.

In killRegistry: 149,  adding subreg.waitFor() should be sufficient.

58: If using a 'for' loop it would be easier to understand if it 
included the usual start, increment and termination.

Instead of burying it in the exception handler.

59, 102, 104: the introduction of the kill boolean makes the test 
harder to understand and seems to be unnecessary.
 the killRegistry() method already will only kill the subprocess 
if it still is alive.


Roger

On 4/2/2018 6:33 AM, Hamlin Li wrote:

would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8188897

webrev: http://cr.openjdk.java.net/~mli/8188897/webrev.00/


Thank you

-Hamlin

Re: RFR of JDK-8188897: java/rmi/registry/reexport/Reexport.java failed with Port already in use

2018-04-08 Thread Hamlin Li


Hi Roger,

I have changed to not use RegistryVM at all, please review the patch: 
http://cr.openjdk.java.net/~mli/8188897/webrev.02/


Thank you

-Hamlin


On 04/04/2018 10:15 PM, Roger Riggs wrote:

Hi Hamlin,

Reexport.java:

I think this change to use a separate process for the 2nd registry 
changes the test so that it does not
address the original test case.  The original problem was the 
incorrect retention of an object in
the object table when the create of a registry in the same process 
failed.
Finally being able to create the registry in the same process assured 
that the object was not

retained in the object table.

Go back to creating the 2nd registry in the test process.


RegistryVM.java: 2, the copyright update should be  "2017, 2018," range.

(I'm  really not a fan of all the RegistryVM methods with the same 
name and minimal and implicit
differences in their functions.  When reading the test, you have to go 
and read the RegistryVM method
to see what it does.  I would have preferred that the full 
createRegistryVM (out, err, options, port) method
was used directly in the tests.  In the case of a method used once, it 
is an inconvenience method, not a convenience).


The new terminate() method is quite similar to the existing cleanup() 
method which does not wait.
It would be a good cleanup to figure out if another method is really 
needed.
The override is going to change the behavior of the existing uses of 
terminate().

It should be checked that it does not break any existing uses.

178: 187: The method comments are not consistent, one says forcibly 
and the other gracefully

but both call requestExit and both call destroy().

Thanks, Roger


On 4/3/2018 11:35 PM, Hamlin Li wrote:

Hi Joe, Roger,

Thank you for reviewing, I have refactored the test more and fix as 
you suggested.


please review http://cr.openjdk.java.net/~mli/8188897/webrev.01/

Thank you

-Hamlin


On 04/04/2018 10:42 AM, Joseph D. Darcy wrote:

Hello,

Some general comments on the coding for tests like this:

* It is preferable to avoid sleep in tests to avoid increasing the 
minimum amount of time a test takes to run. This helps limit the 
overall time it takes the test suite to run.


* If timeouts are used, it is recommend to factor the maximum time 
waited with the jtreg timeout scaling factor; I don't recall its 
exact name. In other words, many of our tests are run on heavily 
loaded systems and the tests take longer than run than on typical 
laptops and workstations so jtreg is invoked with an timeout scaling 
factor. Individual tests should be sensitive to this scaling factor 
for any internal timeout that need to be used.


HTH,

-Joe

On 4/3/2018 7:48 AM, Roger Riggs wrote:

Hi Hamlin,

Instead of a simple time delay, it would be useful to wait for the 
RegistryVM to terminate.

In killRegistry: 149,  adding subreg.waitFor() should be sufficient.

58: If using a 'for' loop it would be easier to understand if it 
included the usual start, increment and termination.

Instead of burying it in the exception handler.

59, 102, 104: the introduction of the kill boolean makes the test 
harder to understand and seems to be unnecessary.
 the killRegistry() method already will only kill the subprocess if 
it still is alive.


Roger

On 4/2/2018 6:33 AM, Hamlin Li wrote:

would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8188897

webrev: http://cr.openjdk.java.net/~mli/8188897/webrev.00/


Thank you

-Hamlin

Re: RFR of JDK-8188897: java/rmi/registry/reexport/Reexport.java failed with Port already in use

2018-04-03 Thread Hamlin Li


Hi Joe, Roger,

Thank you for reviewing, I have refactored the test more and fix as you 
suggested.


please review http://cr.openjdk.java.net/~mli/8188897/webrev.01/

Thank you

-Hamlin


On 04/04/2018 10:42 AM, Joseph D. Darcy wrote:

Hello,

Some general comments on the coding for tests like this:

* It is preferable to avoid sleep in tests to avoid increasing the 
minimum amount of time a test takes to run. This helps limit the 
overall time it takes the test suite to run.


* If timeouts are used, it is recommend to factor the maximum time 
waited with the jtreg timeout scaling factor; I don't recall its exact 
name. In other words, many of our tests are run on heavily loaded 
systems and the tests take longer than run than on typical laptops and 
workstations so jtreg is invoked with an timeout scaling factor. 
Individual tests should be sensitive to this scaling factor for any 
internal timeout that need to be used.


HTH,

-Joe

On 4/3/2018 7:48 AM, Roger Riggs wrote:

Hi Hamlin,

Instead of a simple time delay, it would be useful to wait for the 
RegistryVM to terminate.

In killRegistry: 149,  adding subreg.waitFor() should be sufficient.

58: If using a 'for' loop it would be easier to understand if it 
included the usual start, increment and termination.

Instead of burying it in the exception handler.

59, 102, 104: the introduction of the kill boolean makes the test 
harder to understand and seems to be unnecessary.
 the killRegistry() method already will only kill the subprocess if 
it still is alive.


Roger

On 4/2/2018 6:33 AM, Hamlin Li wrote:

would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8188897

webrev: http://cr.openjdk.java.net/~mli/8188897/webrev.00/


Thank you

-Hamlin

RFR of JDK-8188897: java/rmi/registry/reexport/Reexport.java failed with Port already in use

2018-04-02 Thread Hamlin Li


would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8188897

webrev: http://cr.openjdk.java.net/~mli/8188897/webrev.00/


Thank you

-Hamlin

Re: RFR of 8194284: CheckRegisterInLog.java fails with java.lang.RuntimeException: CheckRegisterInLog got exception timeout 6480000ms out of range



On 19/01/2018 3:09 PM, David Holmes wrote:

Hi Hamlin,

On 19/01/2018 4:28 PM, Hamlin Li wrote:


On 19/01/2018 2:11 PM, David Holmes wrote:

Hi Hamlin,

On 19/01/2018 3:04 PM, Hamlin Li wrote:

Hi Roger, David

Please check the updated webrev at: 
http://cr.openjdk.java.net/~mli/8194284/webrev.00/


RMID.java:

This comment no longer makes sense:

  // when restart rmid, it may take more time than usual because of
  // "port in use" by a possible interloper (check JDK-8168975),
  // so need to set a longer timeout for restart.
! private static final long RESTART_TIMEOUT = 
(long)(TIMEOUT_BASE * 0.9);


Actually I'm not sure it originally made sense - longer than what? 
But as it stands RESTART_TIMEOUT is smaller than TIMEOUT_BASE so the 
comment really seems odd. Perhaps 8168975 will shed some light on 
the intent. ??

Hi David,

The comment still makes sense.
Before 8168975, restart() calls start() which is indeed 
start(STARTTIME_MS), where STARTTIME_MS is 15_000L, but we found out 
in some situation restart() needs more time than start();
So in 8168975, restart() calls start(restartTimeout), where 
restartTimeout is RESTART_TIMEOUT in 8194284.


Okay, then I suggest changing:

 // so need to set a longer timeout for restart.

to

 // so need to set a longer timeout than STARTTIME_MS for restart.

Hi David,

Yes, it's more clear, will push the code with your suggestion.

Thank you
-Hamlin


Thanks,
David



The TestLibrary change looks good.

Do you also think the comment makes sense with my explanation?

Thank you
-Hamlin


Thanks,
David


Thank you

-Hamlin


On 18/01/2018 10:33 PM, Roger Riggs wrote:

Hi Hamlin,

The base bug is that the timeoutFactor is being applied twice, 
once in the RMID constructor
and again in computeDeadline.  It is better to cleanup the 
implementation of the test library
than to fudge the values.  Either respect the timeouts passed in 
(remove the scaling from computeDeadline)

or consistently leave it to the lower level routines. Pick 1.

Thanks, Roger


On 1/18/2018 1:59 AM, Hamlin Li wrote:

Would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8194284

webrev as below.


Thank you

-Hamlin

 



diff -r 0dec8c41170c test/jdk/java/rmi/testlibrary/TestLibrary.java
--- a/test/jdk/java/rmi/testlibrary/TestLibrary.java Wed Jan 17 
20:07:50 2018 -0800
+++ b/test/jdk/java/rmi/testlibrary/TestLibrary.java Thu Jan 18 
14:54:50 2018 +0800

@@ -188,8 +188,12 @@
 public static long computeDeadline(long timestamp, long 
timeout) {

 final long MAX_TIMEOUT_MS = 3_600_000L;

-    if (timeout < 0L || timeout > MAX_TIMEOUT_MS) {
+    if (timeout < 0L) {
 throw new IllegalArgumentException("timeout " + 
timeout + "ms out of range");

+    } else if (timeout > MAX_TIMEOUT_MS) {
+    System.out.format("timeout value(%d) exceeds 
MAX_TIMEOUT_MS(%d), "
+    + "use MAX_TIMEOUT_MS instead!%n", timeout, 
MAX_TIMEOUT_MS);

+    timeout = MAX_TIMEOUT_MS;
 }

 return timestamp + (long)(timeout * getTimeoutFactor());

Re: RFR of 8194284: CheckRegisterInLog.java fails with java.lang.RuntimeException: CheckRegisterInLog got exception timeout 6480000ms out of range



On 19/01/2018 2:11 PM, David Holmes wrote:

Hi Hamlin,

On 19/01/2018 3:04 PM, Hamlin Li wrote:

Hi Roger, David

Please check the updated webrev at: 
http://cr.openjdk.java.net/~mli/8194284/webrev.00/


RMID.java:

This comment no longer makes sense:

  // when restart rmid, it may take more time than usual because of
  // "port in use" by a possible interloper (check JDK-8168975),
  // so need to set a longer timeout for restart.
! private static final long RESTART_TIMEOUT = (long)(TIMEOUT_BASE 
* 0.9);


Actually I'm not sure it originally made sense - longer than what? But 
as it stands RESTART_TIMEOUT is smaller than TIMEOUT_BASE so the 
comment really seems odd. Perhaps 8168975 will shed some light on the 
intent. ??

Hi David,

The comment still makes sense.
Before 8168975, restart() calls start() which is indeed 
start(STARTTIME_MS), where STARTTIME_MS is 15_000L, but we found out in 
some situation restart() needs more time than start();
So in 8168975, restart() calls start(restartTimeout), where 
restartTimeout is RESTART_TIMEOUT in 8194284.


The TestLibrary change looks good.

Do you also think the comment makes sense with my explanation?

Thank you
-Hamlin


Thanks,
David


Thank you

-Hamlin


On 18/01/2018 10:33 PM, Roger Riggs wrote:

Hi Hamlin,

The base bug is that the timeoutFactor is being applied twice, once 
in the RMID constructor
and again in computeDeadline.  It is better to cleanup the 
implementation of the test library
than to fudge the values.  Either respect the timeouts passed in 
(remove the scaling from computeDeadline)

or consistently leave it to the lower level routines.  Pick 1.

Thanks, Roger


On 1/18/2018 1:59 AM, Hamlin Li wrote:

Would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8194284

webrev as below.


Thank you

-Hamlin

 



diff -r 0dec8c41170c test/jdk/java/rmi/testlibrary/TestLibrary.java
--- a/test/jdk/java/rmi/testlibrary/TestLibrary.java    Wed Jan 17 
20:07:50 2018 -0800
+++ b/test/jdk/java/rmi/testlibrary/TestLibrary.java    Thu Jan 18 
14:54:50 2018 +0800

@@ -188,8 +188,12 @@
 public static long computeDeadline(long timestamp, long 
timeout) {

 final long MAX_TIMEOUT_MS = 3_600_000L;

-    if (timeout < 0L || timeout > MAX_TIMEOUT_MS) {
+    if (timeout < 0L) {
 throw new IllegalArgumentException("timeout " + 
timeout + "ms out of range");

+    } else if (timeout > MAX_TIMEOUT_MS) {
+    System.out.format("timeout value(%d) exceeds 
MAX_TIMEOUT_MS(%d), "
+    + "use MAX_TIMEOUT_MS instead!%n", timeout, 
MAX_TIMEOUT_MS);

+    timeout = MAX_TIMEOUT_MS;
 }

 return timestamp + (long)(timeout * getTimeoutFactor());

Re: RFR of 8194284: CheckRegisterInLog.java fails with java.lang.RuntimeException: CheckRegisterInLog got exception timeout 6480000ms out of range


Hi Roger, David

Please check the updated webrev at: 
http://cr.openjdk.java.net/~mli/8194284/webrev.00/


Thank you

-Hamlin


On 18/01/2018 10:33 PM, Roger Riggs wrote:

Hi Hamlin,

The base bug is that the timeoutFactor is being applied twice, once in 
the RMID constructor
and again in computeDeadline.  It is better to cleanup the 
implementation of the test library
than to fudge the values.  Either respect the timeouts passed in 
(remove the scaling from computeDeadline)

or consistently leave it to the lower level routines.  Pick 1.

Thanks, Roger


On 1/18/2018 1:59 AM, Hamlin Li wrote:

Would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8194284

webrev as below.


Thank you

-Hamlin



diff -r 0dec8c41170c test/jdk/java/rmi/testlibrary/TestLibrary.java
--- a/test/jdk/java/rmi/testlibrary/TestLibrary.java    Wed Jan 17 
20:07:50 2018 -0800
+++ b/test/jdk/java/rmi/testlibrary/TestLibrary.java    Thu Jan 18 
14:54:50 2018 +0800

@@ -188,8 +188,12 @@
 public static long computeDeadline(long timestamp, long timeout) {
 final long MAX_TIMEOUT_MS = 3_600_000L;

-    if (timeout < 0L || timeout > MAX_TIMEOUT_MS) {
+    if (timeout < 0L) {
 throw new IllegalArgumentException("timeout " + timeout 
+ "ms out of range");

+    } else if (timeout > MAX_TIMEOUT_MS) {
+    System.out.format("timeout value(%d) exceeds 
MAX_TIMEOUT_MS(%d), "
+    + "use MAX_TIMEOUT_MS instead!%n", timeout, 
MAX_TIMEOUT_MS);

+    timeout = MAX_TIMEOUT_MS;
 }

 return timestamp + (long)(timeout * getTimeoutFactor());

Re: RFR of 8194284: CheckRegisterInLog.java fails with java.lang.RuntimeException: CheckRegisterInLog got exception timeout 6480000ms out of range




On 18/01/2018 5:07 PM, David Holmes wrote:

On 18/01/2018 6:05 PM, Hamlin Li wrote:

On 18/01/2018 3:48 PM, David Holmes wrote:

On 18/01/2018 5:41 PM, Hamlin Li wrote:

Hi David,

Sometime we will run test by command "jtreg -timeoutFactor:xxx 
...", xxx 


Yes but that controls how jtreg manages the execution of the test.

right.

How is that then being used by the tests that call TestDriver?

I assume you mean TestLibrary rather than TestDriver.


Yes - sorry.

If you follow the code in RMID.java, you will find following code, it 
calculates the timeout value by timeoutFactor value.


 long waitTime = (long)(240_000 * 
TestLibrary.getTimeoutFactor());

 restartTimeout = (long)(waitTime * 0.9);
Further if TestDriver limits the timeout to a smaller value, then 
the increase in timeoutFactor won't help at all - people will see a 
timeout, increase the timeoutFactor and still see a timeout!
No, by pass a large timeoutFactor value, I will get test passed 
rather than get timeout exception.


Sorry you are right - the timeout is limited before scaling by the 
timeout-factor, so it will increase. Though it's possible for the 
resulting scaled timeout to still be smaller than the original 
requested one.


But the exception is telling you there is a mismatch between the 
timing expectations of the caller and the actual behaviour in 
TestLibrary. Simply placing a ceiling on the timeout value doesn't fix 
that mismatch.
The patch is not to fix any mismatch, it's just let user to supply a 
large enough value for timeoutFactor (if it's accepted by jtreg) and let 
test pass, it's meaningless to throw an exception.
I don't see any point in computeDeadline imposing an arbitrary maximum 
timeout of 1 hour. (Though it concerns me that tests are trying to set 
timeouts even longer than an hour!)
I neither, but it's history code, currently not necessary to touch it, 
how do you think about it?


Thank you
-Hamlin


David
-



For that matter why is TestDriver even imposing a timeout of its own 
when jtreg imposes an overall timeout?
It's a history topic, if it's necessary we could file another 
enhancement or bug to track it.
In general we have been moving away from test initiated timeouts and 
letting the test harness handle it.
Yes, I agree, that's the reason I refactor it to round down the 
timeout to MAX_TIMEOUT_MS rather than throw an exception by test itself.


Thank you
-Hamlin


Cheers,
David

may be a large number than usual, e.g. 100. The reason we supply a 
large number for timeoutFactor is because we're running test on a 
very slow machine, or we're running test on a docker environment 
which has very limited resource, without a large timeoutFactor 
value, the test will finally get timeout exception. At this 
situation, it's NOT reasonable for user to know the exact 
timeoutFactor by which the calculated result will be 
MAX_TIMEOUT_MS, what's helpful is to let user supply a large enough 
timeoutFactor value and round down the timeout value to 
MAX_TIMEOUT_MS automatically, rather than throw an exception.


Thank you

-Hamlin


On 18/01/2018 3:15 PM, David Holmes wrote:

Hi Hamlin,

This should probably be reviewed on core-libs-dev. I don't think 
jdk-dev is intended for code reviews.


On 18/01/2018 4:59 PM, Hamlin Li wrote:

Would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8194284

webrev as below.


I don't agree with this. Whatever is passing the incorrect timeout 
to the TestLibrary should be fixed. The bug report needs more 
information about where the incorrect value is coming from and why.


Cheers,
David



Thank you

-Hamlin

 



diff -r 0dec8c41170c test/jdk/java/rmi/testlibrary/TestLibrary.java
--- a/test/jdk/java/rmi/testlibrary/TestLibrary.java Wed Jan 17 
20:07:50 2018 -0800
+++ b/test/jdk/java/rmi/testlibrary/TestLibrary.java Thu Jan 18 
14:54:50 2018 +0800

@@ -188,8 +188,12 @@
  public static long computeDeadline(long timestamp, long 
timeout) {

  final long MAX_TIMEOUT_MS = 3_600_000L;

-    if (timeout < 0L || timeout > MAX_TIMEOUT_MS) {
+    if (timeout < 0L) {
  throw new IllegalArgumentException("timeout " + 
timeout + "ms out of range");

+    } else if (timeout > MAX_TIMEOUT_MS) {
+    System.out.format("timeout value(%d) exceeds 
MAX_TIMEOUT_MS(%d), "
+    + "use MAX_TIMEOUT_MS instead!%n", timeout, 
MAX_TIMEOUT_MS);

+    timeout = MAX_TIMEOUT_MS;
  }

  return timestamp + (long)(timeout * getTimeoutFactor());

Re: RFR of 8194284: CheckRegisterInLog.java fails with java.lang.RuntimeException: CheckRegisterInLog got exception timeout 6480000ms out of range




On 18/01/2018 3:48 PM, David Holmes wrote:

On 18/01/2018 5:41 PM, Hamlin Li wrote:

Hi David,

Sometime we will run test by command "jtreg -timeoutFactor:xxx ...", xxx 


Yes but that controls how jtreg manages the execution of the test.

right.

How is that then being used by the tests that call TestDriver?

I assume you mean TestLibrary rather than TestDriver.
If you follow the code in RMID.java, you will find following code, it 
calculates the timeout value by timeoutFactor value.


    long waitTime = (long)(240_000 * TestLibrary.getTimeoutFactor());
    restartTimeout = (long)(waitTime * 0.9);
Further if TestDriver limits the timeout to a smaller value, then the 
increase in timeoutFactor won't help at all - people will see a 
timeout, increase the timeoutFactor and still see a timeout!
No, by pass a large timeoutFactor value, I will get test passed rather 
than get timeout exception.


For that matter why is TestDriver even imposing a timeout of its own 
when jtreg imposes an overall timeout?
It's a history topic, if it's necessary we could file another 
enhancement or bug to track it.
In general we have been moving away from test initiated timeouts and 
letting the test harness handle it.
Yes, I agree, that's the reason I refactor it to round down the timeout 
to MAX_TIMEOUT_MS rather than throw an exception by test itself.


Thank you
-Hamlin


Cheers,
David

may be a large number than usual, e.g. 100. The reason we supply a 
large number for timeoutFactor is because we're running test on a 
very slow machine, or we're running test on a docker environment 
which has very limited resource, without a large timeoutFactor value, 
the test will finally get timeout exception. At this situation, it's 
NOT reasonable for user to know the exact timeoutFactor by which the 
calculated result will be MAX_TIMEOUT_MS, what's helpful is to let 
user supply a large enough timeoutFactor value and round down the 
timeout value to MAX_TIMEOUT_MS automatically, rather than throw an 
exception.


Thank you

-Hamlin


On 18/01/2018 3:15 PM, David Holmes wrote:

Hi Hamlin,

This should probably be reviewed on core-libs-dev. I don't think 
jdk-dev is intended for code reviews.


On 18/01/2018 4:59 PM, Hamlin Li wrote:

Would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8194284

webrev as below.


I don't agree with this. Whatever is passing the incorrect timeout 
to the TestLibrary should be fixed. The bug report needs more 
information about where the incorrect value is coming from and why.


Cheers,
David



Thank you

-Hamlin

 



diff -r 0dec8c41170c test/jdk/java/rmi/testlibrary/TestLibrary.java
--- a/test/jdk/java/rmi/testlibrary/TestLibrary.java    Wed Jan 17 
20:07:50 2018 -0800
+++ b/test/jdk/java/rmi/testlibrary/TestLibrary.java    Thu Jan 18 
14:54:50 2018 +0800

@@ -188,8 +188,12 @@
  public static long computeDeadline(long timestamp, long 
timeout) {

  final long MAX_TIMEOUT_MS = 3_600_000L;

-    if (timeout < 0L || timeout > MAX_TIMEOUT_MS) {
+    if (timeout < 0L) {
  throw new IllegalArgumentException("timeout " + 
timeout + "ms out of range");

+    } else if (timeout > MAX_TIMEOUT_MS) {
+    System.out.format("timeout value(%d) exceeds 
MAX_TIMEOUT_MS(%d), "
+    + "use MAX_TIMEOUT_MS instead!%n", timeout, 
MAX_TIMEOUT_MS);

+    timeout = MAX_TIMEOUT_MS;
  }

  return timestamp + (long)(timeout * getTimeoutFactor());

Re: RFR of 8194284: CheckRegisterInLog.java fails with java.lang.RuntimeException: CheckRegisterInLog got exception timeout 6480000ms out of range

2018-01-17 Thread Hamlin Li


Hi David,

Sometime we will run test by command "jtreg -timeoutFactor:xxx ...", xxx 
may be a large number than usual, e.g. 100. The reason we supply a large 
number for timeoutFactor is because we're running test on a very slow 
machine, or we're running test on a docker environment which has very 
limited resource, without a large timeoutFactor value, the test will 
finally get timeout exception. At this situation, it's NOT reasonable 
for user to know the exact timeoutFactor by which the calculated result 
will be MAX_TIMEOUT_MS, what's helpful is to let user supply a large 
enough timeoutFactor value and round down the timeout value to 
MAX_TIMEOUT_MS automatically, rather than throw an exception.


Thank you

-Hamlin


On 18/01/2018 3:15 PM, David Holmes wrote:

Hi Hamlin,

This should probably be reviewed on core-libs-dev. I don't think 
jdk-dev is intended for code reviews.


On 18/01/2018 4:59 PM, Hamlin Li wrote:

Would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8194284

webrev as below.


I don't agree with this. Whatever is passing the incorrect timeout to 
the TestLibrary should be fixed. The bug report needs more information 
about where the incorrect value is coming from and why.


Cheers,
David



Thank you

-Hamlin



diff -r 0dec8c41170c test/jdk/java/rmi/testlibrary/TestLibrary.java
--- a/test/jdk/java/rmi/testlibrary/TestLibrary.java    Wed Jan 17 
20:07:50 2018 -0800
+++ b/test/jdk/java/rmi/testlibrary/TestLibrary.java    Thu Jan 18 
14:54:50 2018 +0800

@@ -188,8 +188,12 @@
  public static long computeDeadline(long timestamp, long timeout) {
  final long MAX_TIMEOUT_MS = 3_600_000L;

-    if (timeout < 0L || timeout > MAX_TIMEOUT_MS) {
+    if (timeout < 0L) {
  throw new IllegalArgumentException("timeout " + timeout 
+ "ms out of range");

+    } else if (timeout > MAX_TIMEOUT_MS) {
+    System.out.format("timeout value(%d) exceeds 
MAX_TIMEOUT_MS(%d), "
+    + "use MAX_TIMEOUT_MS instead!%n", timeout, 
MAX_TIMEOUT_MS);

+    timeout = MAX_TIMEOUT_MS;
  }

  return timestamp + (long)(timeout * getTimeoutFactor());

Re: (10/jaxp) RFR of JDK-8184062: wrong @modules javax.xml at jaxp/test/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/SurrogatesTest.java

2017-07-10 Thread Hamlin Li


Hi Lance, Amy,

As you suggested, deleted @modules and redundant @test in 
SurrogatesTest.java, the change is pushed.


Thank you

-Hamlin


On 2017/7/10 20:20, Lance Andersen wrote:

Hi Hamilin

On Jul 10, 2017, at 5:59 AM, Amy Lu <mailto:amy...@oracle.com>> wrote:


I noticed the module dependency is declared at 
test/javax/xml/jaxp/unittest/TEST.properties

So @modules in this test file can just be deleted.



Agree and on a quick scan this is the only test in that directory 
which includes @modules



Moreover, just noticed there are two @test which should be cleaned up.

+1


( Not a reviewer.)

Thanks,
Amy

On 7/10/17 5:17 PM, Hamlin Li wrote:

Would you please review below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8184062

webrev: http://cr.openjdk.java.net/~mli/8184062/webrev.00/ 
<http://cr.openjdk.java.net/%7Emli/8184062/webrev.00/>, also attach 
diff as below



Thank you

-Hamlin


diff -r 18b09cba334b 
test/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/SurrogatesTest.java
--- 
a/test/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/SurrogatesTest.java 
 Fri Jul 07 03:13:49 2017 +
+++ 
b/test/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/SurrogatesTest.java 
 Mon Jul 10 02:13:39 2017 -0700

@@ -45,7 +45,7 @@
/*
 * @test
 * @bug 8145974
- * @modules javax.xml
+ * @modules java.xml
 * @test
 * @library /javax/xml/jaxp/libs /javax/xml/jaxp/unittest
 * @run testng/othervm -DrunSecMngr=true 
stream.XMLStreamWriterTest.SurrogatesTest







<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>

(10/jaxp) RFR of JDK-8184062: wrong @modules javax.xml at jaxp/test/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/SurrogatesTest.java

2017-07-10 Thread Hamlin Li


Would you please review below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8184062

webrev: http://cr.openjdk.java.net/~mli/8184062/webrev.00/, also attach 
diff as below



Thank you

-Hamlin


diff -r 18b09cba334b 
test/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/SurrogatesTest.java
--- 
a/test/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/SurrogatesTest.java 
 Fri Jul 07 03:13:49 2017 +
+++ 
b/test/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/SurrogatesTest.java 
 Mon Jul 10 02:13:39 2017 -0700

@@ -45,7 +45,7 @@
 /*
  * @test
  * @bug 8145974
- * @modules javax.xml
+ * @modules java.xml
  * @test
  * @library /javax/xml/jaxp/libs /javax/xml/jaxp/unittest
  * @run testng/othervm -DrunSecMngr=true 
stream.XMLStreamWriterTest.SurrogatesTest

Re: (10) RFR of JDK-8181478, Refactor java/io shell tests to plain java tests

2017-06-18 Thread Hamlin Li




On 2017/6/17 1:31, Paul Sandoz wrote:

On 14 Jun 2017, at 23:29, Hamlin Li  wrote:

Hi Alan, Paul,

Thank you for review, new webrev at: 
http://cr.openjdk.java.net/~mli/8181478/webrev.01/

Please also check my comments inline.


On 2017/6/15 1:28, Alan Bateman wrote:

On 14/06/2017 18:20, Paul Sandoz wrote:

On 12 Jun 2017, at 01:00, Hamlin Li  wrote:

Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8181478

webrev: http://cr.openjdk.java.net/~mli/8181478/webrev.00/


It took me a few moments to grok the NonExistentDriver behaviour. If i got it 
correct, this is checking that deleteOnExit is working correctly?

If so it might be clearer to make this is a separate test that is run as a 
driver and a test with different arguments performing the action and then the 
checking.

Agree, it's not that clear. And I just found out there is another test 
java/io/File/DeleteOnExit.java testing the File.deleteOnExit. so I just remove 
NonExistentDriver.java and related code.

Do you still require the following:

   48 static File nonDir = new File("x.Basic.nonDir");
...
   93 nonDir.delete();
...
  133 if (!nonDir.mkdir()) {
  134 fail(nonDir, "could not create");
  135 }
  136 if (!nonDir.exists() || !nonDir.isDirectory()) {
  137 fail(nonDir, "not created");
  138 }

since it is now just duplicating the assertions from the results of a call to 
show.

Hi Paul,

Thank you for detailed review.
I'm not sure if I understand you correctly, I think it still needs to 
verify the creation of a directory, so I merged the test of dir and 
nonDir in new webrev: http://cr.openjdk.java.net/~mli/8181478/webrev.02/


Thank you
-Hamlin


Paul.


I agree. Also in FileOpenTest then it can use APIs to set the hidden attribute, 
no need to launch attrib.exe each time.

This is a test running only on windows.
I'm not sure if I understand you correctly. In new patch I use Files.setAttribute(path, 
"dos:hidden", true/false); to hide/un-hide a file, and use File.setReadOnly() 
to make a file read only.

Thank you
-Hamlin

-Alan

Re: (10) RFR of JDK-8181912,Refactor locale related shell test test/java/io/File/MacPathTest.sh to java test

2017-06-16 Thread Hamlin Li

Please check new webrev: http://cr.openjdk.java.net/~mli/8181912/webrev.01/

I updated as Felix suggest to pass "LC_ALL" into test process as an
environment by using ProcessTools

Thank you

-Hamlin

On 2017/6/16 17:14, Felix Yang wrote:

Hamlin,

this may be not a blocker. if you prefer 'pure' java, it can be
achieved by adjusting environment for ProcessBuilder.

see Amy's patch:
http://cr.openjdk.java.net/~amlu/8181395/webrev.00/test/java/nio/charset/Charset/DefaultCharsetTest.java.html

In further, you may extend and wrap such usage in ProcessTools,
because this looks to be a common usage, at least in encoding use cases.

-Felix
On 2017/6/16 11:40, Hamlin Li wrote:

Hi Felix,

Thank you for your suggestions.

But I think it's just a java wrapper around a shell, not a real java.

Thank you

-Hamlin

On 2017/6/16 9:41, Felix Yang wrote:

Hi Hamlin,

I think you need something like:

/ProcessTools.executeCommand("sh", "-c", yourTestCmd).../

yourTestCmd is like the original cmd in shell "
export LC_ALL=en_US.UTF-8 ;${TESTJAVA}/bin/java ${TESTVMOPTS} -cp ${TESTCLASSES}
MacPathTest"
-Felix
On 2017/6/16 9:32, Naoto Sato wrote:

Hi Hamlin,

What I meant was that setting the java locale either through
Locale.setDefault() or user.language/user.country properties won't
affect the default encoding determination. Other properties
(file.encoding/sun.jnu.encoding) would set the default, but I am
not sure how they are supposed to be used in regression tests.

Naoto

On 6/15/17 4:59 PM, Hamlin Li wrote:

Hi Naoto,

Thank you for comments.

Do you mean there is no way to set encoding through system
property or java API? And can I understand it as it's better to
keep the shell test rather than convert it to java test.

Thank you

-Hamlin

On 2017/6/16 0:45, Naoto Sato wrote:

Hi,

Setting the default Java Locale and/or user.* properties has
nothing to do with the default encoding. The default encoding on
mac/unix environments is determined from the environment variable
LC_CTYPE.

Naoto

On 6/14/17 8:35 PM, Hamlin Li wrote:

On 2017/6/15 1:22, Alan Bateman wrote:

On 12/06/2017 09:00, Hamlin Li wrote:

Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8181912

webrev: http://cr.openjdk.java.net/~mli/8181912/webrev.00/

Are you sure that setting the user.* properties on the command
actually works? I assume we'll have to change this back at some
point to launch the test with LC_ALL set to UTF-8.

Hi Alan,

Besides of setting user.language and user.country, the patch
also sets file.encoding, and sun.jnu.encoding as UTF-8.

I understand your concern: we're not sure if setting -Dxxx
properties will have totally same functionality as setting env
variable LC_ALL. I have no answer for this question.

What tests have been done:
1. set user.* properties will affect what Locale.getDefault()
gets, set file.encoding will affect what
java.nio.charset.Charset.defaultCharset() gets.

2. jprt passed.
3. test on some other mac machine in SQE pool.

Do you suggest we should keep this test as shell?

Thank you
-Hamlin

-Alan

Re: (10) RFR of JDK-8181912,Refactor locale related shell test test/java/io/File/MacPathTest.sh to java test

2017-06-16 Thread Hamlin Li

On 2017/6/16 17:14, Felix Yang wrote:

Hamlin,

this may be not a blocker. if you prefer 'pure' java, it can be
achieved by adjusting environment for ProcessBuilder.

see Amy's patch:
http://cr.openjdk.java.net/~amlu/8181395/webrev.00/test/java/nio/charset/Charset/DefaultCharsetTest.java.html

In further, you may extend and wrap such usage in ProcessTools,
because this looks to be a common usage, at least in encoding use cases.

Good point. Thank you Felix, I will do it.

Thank you
-Hamlin

-Felix
On 2017/6/16 11:40, Hamlin Li wrote:

Hi Felix,

Thank you for your suggestions.

But I think it's just a java wrapper around a shell, not a real java.

Thank you

-Hamlin

On 2017/6/16 9:41, Felix Yang wrote:

Hi Hamlin,

I think you need something like:

/ProcessTools.executeCommand("sh", "-c", yourTestCmd).../

yourTestCmd is like the original cmd in shell "
export LC_ALL=en_US.UTF-8 ;${TESTJAVA}/bin/java ${TESTVMOPTS} -cp ${TESTCLASSES}
MacPathTest"
-Felix
On 2017/6/16 9:32, Naoto Sato wrote:

Hi Hamlin,

Naoto

On 6/15/17 4:59 PM, Hamlin Li wrote:

Hi Naoto,

Thank you for comments.

Do you mean there is no way to set encoding through system
property or java API? And can I understand it as it's better to
keep the shell test rather than convert it to java test.

Thank you

-Hamlin

On 2017/6/16 0:45, Naoto Sato wrote:

Hi,

Naoto

On 6/14/17 8:35 PM, Hamlin Li wrote:

On 2017/6/15 1:22, Alan Bateman wrote:

On 12/06/2017 09:00, Hamlin Li wrote:

Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8181912

webrev: http://cr.openjdk.java.net/~mli/8181912/webrev.00/

Are you sure that setting the user.* properties on the command
actually works? I assume we'll have to change this back at some
point to launch the test with LC_ALL set to UTF-8.

Hi Alan,

Besides of setting user.language and user.country, the patch
also sets file.encoding, and sun.jnu.encoding as UTF-8.

I understand your concern: we're not sure if setting -Dxxx
properties will have totally same functionality as setting env
variable LC_ALL. I have no answer for this question.

What tests have been done:
1. set user.* properties will affect what Locale.getDefault()
gets, set file.encoding will affect what
java.nio.charset.Charset.defaultCharset() gets.

2. jprt passed.
3. test on some other mac machine in SQE pool.

Do you suggest we should keep this test as shell?

Thank you
-Hamlin

-Alan

Re: (10) RFR of JDK-8181478, Refactor java/io shell tests to plain java tests

2017-06-16 Thread Hamlin Li


Ping.

Thank you

-Hamlin


On 2017/6/15 14:29, Hamlin Li wrote:

Hi Alan, Paul,

Thank you for review, new webrev at: 
http://cr.openjdk.java.net/~mli/8181478/webrev.01/


Please also check my comments inline.


On 2017/6/15 1:28, Alan Bateman wrote:


On 14/06/2017 18:20, Paul Sandoz wrote:

On 12 Jun 2017, at 01:00, Hamlin Li  wrote:

Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8181478

webrev: http://cr.openjdk.java.net/~mli/8181478/webrev.00/

It took me a few moments to grok the NonExistentDriver behaviour. If 
i got it correct, this is checking that deleteOnExit is working 
correctly?


If so it might be clearer to make this is a separate test that is 
run as a driver and a test with different arguments performing the 
action and then the checking.
Agree, it's not that clear. And I just found out there is another test 
java/io/File/DeleteOnExit.java testing the File.deleteOnExit. so I 
just remove NonExistentDriver.java and related code.
I agree. Also in FileOpenTest then it can use APIs to set the hidden 
attribute, no need to launch attrib.exe each time.

This is a test running only on windows.
I'm not sure if I understand you correctly. In new patch I use 
Files.setAttribute(path, "dos:hidden", true/false); to hide/un-hide a 
file, and use File.setReadOnly() to make a file read only.


Thank you
-Hamlin


-Alan

Re: (10) RFR of JDK-8181912,Refactor locale related shell test test/java/io/File/MacPathTest.sh to java test

2017-06-15 Thread Hamlin Li


Hi Felix,

Thank you for your suggestions.

But I think it's just a java wrapper around a shell, not a real java.

Thank you

-Hamlin


On 2017/6/16 9:41, Felix Yang wrote:


Hi Hamlin,

 I think you need something like:

/ProcessTools.executeCommand("sh", "-c", yourTestCmd).../

yourTestCmd is like the original cmd in shell "
export LC_ALL=en_US.UTF-8 ;${TESTJAVA}/bin/java ${TESTVMOPTS} -cp ${TESTCLASSES} 
MacPathTest"
-Felix
On 2017/6/16 9:32, Naoto Sato wrote:

Hi Hamlin,

What I meant was that setting the java locale either through 
Locale.setDefault() or user.language/user.country properties won't 
affect the default encoding determination. Other properties 
(file.encoding/sun.jnu.encoding) would set the default, but I am not 
sure how they are supposed to be used in regression tests.


Naoto

On 6/15/17 4:59 PM, Hamlin Li wrote:

Hi Naoto,

Thank you for comments.

Do you mean there is no way to set encoding through system property 
or java API? And can I understand it as it's better to keep the 
shell test rather than convert it to java test.


Thank you

-Hamlin


On 2017/6/16 0:45, Naoto Sato wrote:

Hi,

Setting the default Java Locale and/or user.* properties has 
nothing to do with the default encoding. The default encoding on 
mac/unix environments is determined from the environment variable 
LC_CTYPE.


Naoto

On 6/14/17 8:35 PM, Hamlin Li wrote:


On 2017/6/15 1:22, Alan Bateman wrote:

On 12/06/2017 09:00, Hamlin Li wrote:

Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8181912

webrev: http://cr.openjdk.java.net/~mli/8181912/webrev.00/

Are you sure that setting the user.* properties on the command 
actually works? I assume we'll have to change this back at some 
point to launch the test with LC_ALL set to UTF-8.

Hi Alan,

Besides of setting user.language and user.country, the patch also 
sets file.encoding, and sun.jnu.encoding as UTF-8.


I understand your concern: we're not sure if setting -Dxxx 
properties will have totally same functionality as setting env 
variable LC_ALL. I have no answer for this question.

What tests have been done:
  1. set user.* properties will affect what Locale.getDefault() 
gets, set file.encoding will affect what 
java.nio.charset.Charset.defaultCharset() gets.

  2. jprt passed.
  3. test on some other mac machine in SQE pool.

Do you suggest we should keep this test as shell?

Thank you
-Hamlin


-Alan

Re: (10) RFR of JDK-8181912,Refactor locale related shell test test/java/io/File/MacPathTest.sh to java test

2017-06-15 Thread Hamlin Li


Hi Naoto,

Thank you for comments.

By searching I found out others have the similar issues when trying to 
replace LC_ALL with file.encoding/sun.jnu.encoding, they're accessing 
file names. That means LC_ALL is IMPORTANT. So I think it's better for 
me to keep the shell test for now.


Thank you

-Hamlin


On 2017/6/16 9:32, Naoto Sato wrote:

Hi Hamlin,

What I meant was that setting the java locale either through 
Locale.setDefault() or user.language/user.country properties won't 
affect the default encoding determination. Other properties 
(file.encoding/sun.jnu.encoding) would set the default, but I am not 
sure how they are supposed to be used in regression tests.


Naoto

On 6/15/17 4:59 PM, Hamlin Li wrote:

Hi Naoto,

Thank you for comments.

Do you mean there is no way to set encoding through system property 
or java API? And can I understand it as it's better to keep the shell 
test rather than convert it to java test.


Thank you

-Hamlin


On 2017/6/16 0:45, Naoto Sato wrote:

Hi,

Setting the default Java Locale and/or user.* properties has nothing 
to do with the default encoding. The default encoding on mac/unix 
environments is determined from the environment variable LC_CTYPE.


Naoto

On 6/14/17 8:35 PM, Hamlin Li wrote:


On 2017/6/15 1:22, Alan Bateman wrote:

On 12/06/2017 09:00, Hamlin Li wrote:

Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8181912

webrev: http://cr.openjdk.java.net/~mli/8181912/webrev.00/

Are you sure that setting the user.* properties on the command 
actually works? I assume we'll have to change this back at some 
point to launch the test with LC_ALL set to UTF-8.

Hi Alan,

Besides of setting user.language and user.country, the patch also 
sets file.encoding, and sun.jnu.encoding as UTF-8.


I understand your concern: we're not sure if setting -Dxxx 
properties will have totally same functionality as setting env 
variable LC_ALL. I have no answer for this question.

What tests have been done:
  1. set user.* properties will affect what Locale.getDefault() 
gets, set file.encoding will affect what 
java.nio.charset.Charset.defaultCharset() gets.

  2. jprt passed.
  3. test on some other mac machine in SQE pool.

Do you suggest we should keep this test as shell?

Thank you
-Hamlin


-Alan

Re: (10) RFR of JDK-8181912,Refactor locale related shell test test/java/io/File/MacPathTest.sh to java test

2017-06-15 Thread Hamlin Li


Hi Naoto,

Thank you for comments.

Do you mean there is no way to set encoding through system property or 
java API? And can I understand it as it's better to keep the shell test 
rather than convert it to java test.


Thank you

-Hamlin


On 2017/6/16 0:45, Naoto Sato wrote:

Hi,

Setting the default Java Locale and/or user.* properties has nothing 
to do with the default encoding. The default encoding on mac/unix 
environments is determined from the environment variable LC_CTYPE.


Naoto

On 6/14/17 8:35 PM, Hamlin Li wrote:


On 2017/6/15 1:22, Alan Bateman wrote:

On 12/06/2017 09:00, Hamlin Li wrote:

Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8181912

webrev: http://cr.openjdk.java.net/~mli/8181912/webrev.00/

Are you sure that setting the user.* properties on the command 
actually works? I assume we'll have to change this back at some 
point to launch the test with LC_ALL set to UTF-8.

Hi Alan,

Besides of setting user.language and user.country, the patch also 
sets file.encoding, and sun.jnu.encoding as UTF-8.


I understand your concern: we're not sure if setting -Dxxx properties 
will have totally same functionality as setting env variable LC_ALL. 
I have no answer for this question.

What tests have been done:
  1. set user.* properties will affect what Locale.getDefault() gets, 
set file.encoding will affect what 
java.nio.charset.Charset.defaultCharset() gets.

  2. jprt passed.
  3. test on some other mac machine in SQE pool.

Do you suggest we should keep this test as shell?

Thank you
-Hamlin


-Alan

Re: (10) RFR of JDK-8181478, Refactor java/io shell tests to plain java tests

2017-06-14 Thread Hamlin Li


Hi Alan, Paul,

Thank you for review, new webrev at: 
http://cr.openjdk.java.net/~mli/8181478/webrev.01/


Please also check my comments inline.


On 2017/6/15 1:28, Alan Bateman wrote:


On 14/06/2017 18:20, Paul Sandoz wrote:

On 12 Jun 2017, at 01:00, Hamlin Li  wrote:

Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8181478

webrev: http://cr.openjdk.java.net/~mli/8181478/webrev.00/

It took me a few moments to grok the NonExistentDriver behaviour. If 
i got it correct, this is checking that deleteOnExit is working 
correctly?


If so it might be clearer to make this is a separate test that is run 
as a driver and a test with different arguments performing the action 
and then the checking.
Agree, it's not that clear. And I just found out there is another test 
java/io/File/DeleteOnExit.java testing the File.deleteOnExit. so I just 
remove NonExistentDriver.java and related code.
I agree. Also in FileOpenTest then it can use APIs to set the hidden 
attribute, no need to launch attrib.exe each time.

This is a test running only on windows.
I'm not sure if I understand you correctly. In new patch I use 
Files.setAttribute(path, "dos:hidden", true/false); to hide/un-hide a 
file, and use File.setReadOnly() to make a file read only.


Thank you
-Hamlin


-Alan

Re: (10) RFR of JDK-8181912,Refactor locale related shell test test/java/io/File/MacPathTest.sh to java test

2017-06-14 Thread Hamlin Li



On 2017/6/15 1:22, Alan Bateman wrote:

On 12/06/2017 09:00, Hamlin Li wrote:

Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8181912

webrev: http://cr.openjdk.java.net/~mli/8181912/webrev.00/

Are you sure that setting the user.* properties on the command 
actually works? I assume we'll have to change this back at some point 
to launch the test with LC_ALL set to UTF-8.

Hi Alan,

Besides of setting user.language and user.country, the patch also sets 
file.encoding, and sun.jnu.encoding as UTF-8.


I understand your concern: we're not sure if setting -Dxxx properties 
will have totally same functionality as setting env variable LC_ALL. I 
have no answer for this question.

What tests have been done:
 1. set user.* properties will affect what Locale.getDefault() gets, 
set file.encoding will affect what 
java.nio.charset.Charset.defaultCharset() gets.

 2. jprt passed.
 3. test on some other mac machine in SQE pool.

Do you suggest we should keep this test as shell?

Thank you
-Hamlin


-Alan

Re: (10) RFR of JDK-8179242, OutOfMemoryError in java/util/Arrays/ParallelPrefix.java

2017-06-14 Thread Hamlin Li




Also disable to run ParallelPrefix.java in concurrency.


Ok.

Please check the new webrev: http://cr.openjdk.java.net/~mli/8179242/webrev.01/


   29  * @run testng/othervm -Xms256m -Xmx1024m ParallelPrefix

Why are you setting the max heap size?

In my test result, 1024M is sufficient, and if I don't set it, I'm not sure 
what's the default value for it, so set it explicitly for this memory sensitive 
test.
Please let me know if you think it's unnecessary.


Hi Paul,

It just feels a little odd. Given that failed test were reported for machines 
with much more than 2g of memory. Implying there is something else going on, 
perhaps perturbed by other changes of test infrastructure?
Yes. 2g is just a defensive check in case the test is  running on an 
system without large memory. Besides of this, there is another 
protection: disable ParallelPrefix.java to run in concurrency, so the 
test will not compete with other tests for memory. If it still fails in 
the future, we can remove large size array test unconditionally or check 
getFreePhysicalMemorySize() at runtime.

I think what you have is ok.

Thank you for review, I will push the code.

Thank you
-Hamlin


Paul.

Re: (10) RFR of JDK-8181912,Refactor locale related shell test test/java/io/File/MacPathTest.sh to java test

2017-06-13 Thread Hamlin Li


Ping.

Thank you

-Hamlin


On 2017/6/12 16:00, Hamlin Li wrote:

Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8181912

webrev: http://cr.openjdk.java.net/~mli/8181912/webrev.00/


Thank you

-Hamlin

Re: (10) RFR of JDK-8179242, OutOfMemoryError in java/util/Arrays/ParallelPrefix.java

2017-06-13 Thread Hamlin Li


Hi Paul,

Please check my comments inline below.


On 2017/6/14 1:23, Paul Sandoz wrote:

On 12 Jun 2017, at 20:37, Hamlin Li  wrote:

Hi Paul,

Good idea. Although the downside is the test will depend on java.management and 
jdk.management modules, I think it's acceptable.


Although to Roger’s point is there a simpler way?

No. Please also check my response to Roger.

Also disable to run ParallelPrefix.java in concurrency.


Ok.

Please check the new webrev: http://cr.openjdk.java.net/~mli/8179242/webrev.01/


   29  * @run testng/othervm -Xms256m -Xmx1024m ParallelPrefix

Why are you setting the max heap size?
In my test result, 1024M is sufficient, and if I don't set it, I'm not 
sure what's the default value for it, so set it explicitly for this 
memory sensitive test.

Please let me know if you think it's unnecessary.

Thank you
-Hamlin


Paul.

Re: (10) RFR of JDK-8179242, OutOfMemoryError in java/util/Arrays/ParallelPrefix.java

2017-06-13 Thread Hamlin Li


Hi Roger,

Runtime.maxMemory() is for the memory JVM is trying to use, e.g. if you 
set -Xmx128M then you get 128M.


But what we try to do is to avoid running large array size test when 
total physical memory of system is lower than 2G, to get total physical 
memory I think OperatingSystemMXBean is necessary.


Thank you

-Hamlin


On 2017/6/13 21:55, Roger Riggs wrote:

Hi Hamlin,

For max memory is Runtime.maxMemory insufficient?

Thanks, Roger

On 6/12/2017 11:37 PM, Hamlin Li wrote:

Hi Paul,

Good idea. Although the downside is the test will depend on 
java.management and jdk.management modules, I think it's acceptable.


Also disable to run ParallelPrefix.java in concurrency.

Please check the new webrev: 
http://cr.openjdk.java.net/~mli/8179242/webrev.01/


Thank you

-Hamlin

On 2017/6/13 0:06, Paul Sandoz wrote:

Hi Hamlin,

Do you know if it is possible to adjust the array sizes based on max 
memory thresholds?


For example in the os.maxMemory is less than 2g then omit the 
LARGE_ARRAY_SIZE from the test (assuming that reduces the memory 
pressure, which it likely should).


Paul.


On 12 Jun 2017, at 02:38, Hamlin Li  wrote:

Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8179242

webrev: http://cr.openjdk.java.net/~mli/8179242/webrev.00/

Thank you

-Hamlin

 



the test takes about 700M on linux 64, so propose to fix the issue 
with below patch:


diff -r 9200df3d3c0b test/java/util/Arrays/ParallelPrefix.java
--- a/test/java/util/Arrays/ParallelPrefix.javaSun Jun 11 
18:36:23 2017 -0700
+++ b/test/java/util/Arrays/ParallelPrefix.javaMon Jun 12 
02:35:20 2017 -0700

@@ -25,7 +25,8 @@
  * @test 8014076 8025067
  * @summary unit test for Arrays.ParallelPrefix().
  * @author Tristan Yan
- * @run testng ParallelPrefix
+ * @requires os.maxMemory >= 2g
+ * @run testng/othervm -Xms512m -Xmx1024m ParallelPrefix
  */

Re: (10) RFR of JDK-8179242, OutOfMemoryError in java/util/Arrays/ParallelPrefix.java


Hi Paul,

Good idea. Although the downside is the test will depend on 
java.management and jdk.management modules, I think it's acceptable.


Also disable to run ParallelPrefix.java in concurrency.

Please check the new webrev: 
http://cr.openjdk.java.net/~mli/8179242/webrev.01/


Thank you

-Hamlin

On 2017/6/13 0:06, Paul Sandoz wrote:

Hi Hamlin,

Do you know if it is possible to adjust the array sizes based on max memory 
thresholds?

For example in the os.maxMemory is less than 2g then omit the LARGE_ARRAY_SIZE 
from the test (assuming that reduces the memory pressure, which it likely 
should).

Paul.


On 12 Jun 2017, at 02:38, Hamlin Li  wrote:

Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8179242

webrev: http://cr.openjdk.java.net/~mli/8179242/webrev.00/

Thank you

-Hamlin



the test takes about 700M on linux 64, so propose to fix the issue with below 
patch:

diff -r 9200df3d3c0b test/java/util/Arrays/ParallelPrefix.java
--- a/test/java/util/Arrays/ParallelPrefix.javaSun Jun 11 18:36:23 2017 
-0700
+++ b/test/java/util/Arrays/ParallelPrefix.javaMon Jun 12 02:35:20 2017 
-0700
@@ -25,7 +25,8 @@
  * @test 8014076 8025067
  * @summary unit test for Arrays.ParallelPrefix().
  * @author Tristan Yan
- * @run testng ParallelPrefix
+ * @requires os.maxMemory >= 2g
+ * @run testng/othervm -Xms512m -Xmx1024m ParallelPrefix
  */

(10) RFR of JDK-8179242, OutOfMemoryError in java/util/Arrays/ParallelPrefix.java


Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8179242

webrev: http://cr.openjdk.java.net/~mli/8179242/webrev.00/

Thank you

-Hamlin



the test takes about 700M on linux 64, so propose to fix the issue with 
below patch:


diff -r 9200df3d3c0b test/java/util/Arrays/ParallelPrefix.java
--- a/test/java/util/Arrays/ParallelPrefix.javaSun Jun 11 18:36:23 
2017 -0700
+++ b/test/java/util/Arrays/ParallelPrefix.javaMon Jun 12 02:35:20 
2017 -0700

@@ -25,7 +25,8 @@
  * @test 8014076 8025067
  * @summary unit test for Arrays.ParallelPrefix().
  * @author Tristan Yan
- * @run testng ParallelPrefix
+ * @requires os.maxMemory >= 2g
+ * @run testng/othervm -Xms512m -Xmx1024m ParallelPrefix
  */

(10) RFR of JDK-8181912,Refactor locale related shell test test/java/io/File/MacPathTest.sh to java test


Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8181912

webrev: http://cr.openjdk.java.net/~mli/8181912/webrev.00/


Thank you

-Hamlin

(10) RFR of JDK-8181478,Refactor java/io shell tests to plain java tests


Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8181478

webrev: http://cr.openjdk.java.net/~mli/8181478/webrev.00/


Thank you

-Hamlin

Re: RFR(M) : 8181761: add explicit @build actions for jdk.test.lib classes in all :tier2 tests



On 2017/6/9 12:19, Igor Ignatyev wrote:

Hi Hamlin,

Hi Igor,

Thank you for explanation.
I'm going to add explicit @build for jdk.test.lib.** classes in jdk 
tests which are in :tier[1-3].

Got it.
unfortunately this approach does not work if a test depends on a 
library indirectly, e.g. you might have a couple of tests which share 
some code (but not from /test/lib or /jdk/test/lib/testlibrary) and 
this code depends on a testlibrary, so you will need to analyze test 
classes files, which is not much different from analyzing them to get 
all explicit @build.
since your approach works only if all testlibrary classes are in a 
package, we will need to update lots of test which use classes from 
default package, which makes it a bit more painful.

I see. Yes, it's lot of extra clean-up of test library.
but the main disadvantage is the assumption that all tests must follow 
this rule, and if one test does not follow it, this test might pass, 
while other innocent tests will fail.
I agree, the most failing tests are not the root cause but just victim, 
that's the reason to clean up all tests not just "fix" the failing ones.
 if we were considering approaches w/ such flaw, I'd strongly 
recommend to simply remove all @build actions (except needed due to 
reflection accesses) and have the straightforward rule to follow and 
one-liner to check this rule.
Got it. Maybe the most thorough/simple solution is to just separate 
tests output from each other totally.


Thank you for
-Hamlin


-- Igor
On Jun 8, 2017, at 7:58 PM, Hamlin Li <mailto:huaming...@oracle.com>> wrote:


Hi Igor,

I'm coping Jon as it needs Jon's comments.


Thank you for doing such a great refactoring, I believe it will make 
tests run more stable.


I saw you are adding explicit @build to lots of test, are you going 
to clean up all tests to add explicit @build? If the answer is "yes", 
then I have a possible simple solution for you to consider.


Steps:

  1. refactor all test library classes in unnamed packages to named 
package.


  2. just need to add explicit @build for every "import x.y.z;" in a 
test, means you only need to add @build for every test lib class 
directly used by your test, lib classes' dependency is not needed. 
e.g. there is a test containing only "import 
jdk.test.lib.process.ProcessTools;", then only need to add "@build 
jdk.test.lib.process.ProcessTools", and no @build is needed to be 
added for jdk.test.lib.process.StreamPumper, 
jdk.test.lib.JDKToolFinder and jdk.test.lib.Utils and further 
dependency if they have.


This solution is based on the situation that all tests will be added 
@build for all test lib classes they directly used, that means no 
test is allow to just add "@library xxx".


The advantage of this solution is:

  1. the rule is simple to follow, reduce the pain for engineer 
writing the tests.


  2. it might be possible to do it automatically, and have a tool to 
monitor all checked-in test code by this rule;


  3. no need to refactor the test lib too much, e.g. no need to 
remove the dependency between test lib packages;



Of course, Jon's original strict solution is absolutely correct: to 
add @build for all test lib classes even if for the implicitly 
dependent classes. Jon's strict solution is for a more complicated 
situation where some tests are using explicit @build, but others are 
not, in this mixed situation my solution will *NOT* work. But as 
you're cleaning up all the tests, I think it's not necessary to 
follow such a strict rule.



I might miss something, please correct me then.


Thank you

-Hamlin


On 2017/6/8 15:20, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8181761/webrev.00/index.html

432 lines changed: 404 ins; 1 del; 27 mod;

Hi all,

could you please review this changeset which adds explicit @build actions to 
tier2 jdk tests? other tests will be updated by the corresponding sub-tasks of 
8181758[1].

webrev:http://cr.openjdk.java.net/~iignatyev//8181761/webrev.00/index.html
jbs:https://bugs.openjdk.java.net/browse/JDK-8181761
testing: :tier2 (in progress)

[1]https://bugs.openjdk.java.net/browse/JDK-8181758

Thanks,
-- Igor

Re: JDK 10 RFR of JDK-8181395: Refactor several java/nio locale related shell tests to java


Got it, thank you for explanation.

Although I don't think it's necessary, it's harmless to add the check.

Thank you

-Hamlin


On 2017/6/9 11:10, Amy Lu wrote:

Thank you Hamlin.

I actually think -Dfile.encoding=UTF-8 -Dsun.jnu.encoding=UTF-8 is 
enough. I added Locale.setDefault(Locale.US) and restore "just for safe".
The encoding check (and warnings/usage) is mainly for helping one who 
tries to run it from command line.


Thanks,
Amy

On 6/9/17 10:14 AM, Hamlin Li wrote:

Hi Amy,

For refactoring MacPathTest, I wonder if it's sufficient to just 
change below line?



- * @run shell MacPathTest.sh
+ * @run main/othervm -Duser.language=en -Duser.country=US 
-Dfile.encoding=UTF-8 -Dsun.jnu.encoding=UTF-8 MacPathTest

  */

Thank you

-Hamlin


On 2017/6/7 14:14, Amy Lu wrote:

java/nio/charset/Charset/default.sh
java/nio/charset/coders/CheckSJISMappingProp.sh
java/nio/file/Path/MacPathTest.java which runs shell test:
 * @run shell MacPathTest.sh

Please review this patch to refactor these shell tests to java.

Note that in MacPathTest.sh, it requires the test to be run with 
LC_ALL=en_US.UTF-8 env. But unlike the other two tests, LC_ALL 
settings here is just to setup an env in order for the test to work 
correctly, it’s not the locale it intends to test.


bug: https://bugs.openjdk.java.net/browse/JDK-8181395
webrev: http://cr.openjdk.java.net/~amlu/8181395/webrev.00/

Thanks,
Amy

Re: RFR(M) : 8181761: add explicit @build actions for jdk.test.lib classes in all :tier2 tests


Hi Igor,

I'm coping Jon as it needs Jon's comments.


Thank you for doing such a great refactoring, I believe it will make 
tests run more stable.


I saw you are adding explicit @build to lots of test, are you going to 
clean up all tests to add explicit @build? If the answer is "yes", then 
I have a possible simple solution for you to consider.


Steps:

  1. refactor all test library classes in unnamed packages to named 
package.


  2. just need to add explicit @build for every "import x.y.z;" in a 
test, means you only need to add @build for every test lib class 
directly used by your test, lib classes' dependency is not needed. e.g. 
there is a test containing only "import 
jdk.test.lib.process.ProcessTools;", then only need to add "@build 
jdk.test.lib.process.ProcessTools", and no @build is needed to be added 
for jdk.test.lib.process.StreamPumper, jdk.test.lib.JDKToolFinder and 
jdk.test.lib.Utils and further dependency if they have.


This solution is based on the situation that all tests will be added 
@build for all test lib classes they directly used, that means no test 
is allow to just add "@library xxx".


The advantage of this solution is:

  1. the rule is simple to follow, reduce the pain for engineer writing 
the tests.


  2. it might be possible to do it automatically, and have a tool to 
monitor all checked-in test code by this rule;


  3. no need to refactor the test lib too much, e.g. no need to remove 
the dependency between test lib packages;



Of course, Jon's original strict solution is absolutely correct: to add 
@build for all test lib classes even if for the implicitly dependent 
classes. Jon's strict solution is for a more complicated situation where 
some tests are using explicit @build, but others are not, in this mixed 
situation my solution will *NOT* work. But as you're cleaning up all the 
tests, I think it's not necessary to follow such a strict rule.



I might miss something, please correct me then.


Thank you

-Hamlin


On 2017/6/8 15:20, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8181761/webrev.00/index.html

432 lines changed: 404 ins; 1 del; 27 mod;

Hi all,

could you please review this changeset which adds explicit @build actions to 
tier2 jdk tests? other tests will be updated by the corresponding sub-tasks of 
8181758[1].

webrev: http://cr.openjdk.java.net/~iignatyev//8181761/webrev.00/index.html
jbs: https://bugs.openjdk.java.net/browse/JDK-8181761
testing: :tier2 (in progress)

[1] https://bugs.openjdk.java.net/browse/JDK-8181758

Thanks,
-- Igor

Re: JDK 10 RFR of JDK-8181395: Refactor several java/nio locale related shell tests to java