RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently
Leonid Mesnik
leonid.mesnik at oracle.com
Mon Nov 25 09:24:52 PST 2013
Hi
I have a couple of high-level questions.
On 11/25/2013 08:28 PM, Mattias Tobiasson wrote:
> Hi,
> The test has been updated after the first review.
> The two java files for each test has been merged to a single file.
>
>
> Updated summary of changes:
> 1. The real test bug fix is to add flag "-Xshare:off" when starting the "Application" instance. Without that flag, the test for ClassFileTransformer in RedefineAgent.java fails intermittently. The flag is added in function startApplication() in RunnerUtil.java.
What is the reason of this intermittent failure?
>
> 2. Removed all bash scripts.
Great!
>
> 3. Merged the setup bash script and the java test code for each test to a single java file. The old java test code is unchanged, it has just been moved to static nested class in the new java setup file.
>
> 4. Added some utility functions to jdk/testlibrary.
Here some comments about library code.
http://cr.openjdk.java.net/~miauno/6461635/webrev.01/jdk/test/lib/testlibrary/jdk/testlibrary/ProcessThread.java.udiff.html
+ try {
+ output = new OutputAnalyzer(this.process);
+ } catch (Throwable t) {
+ String name = Thread.currentThread().getName();
+ System.out.println(String.format("ProcessThread[%s] failed: %s", name, t.toString()));
+ throw t;
+ } finally {
+ String logMsg = ProcessTools.getProcessLog(processBuilder, output);
+ System.out.println(logMsg);
+ }
You used output in finally which is not initialized properly in the case
of Exception.
Could we get another uncaught exception in finally?
http://cr.openjdk.java.net/~miauno/6461635/webrev.01/jdk/test/lib/testlibrary/jdk/testlibrary/ProcessTools.java.udiff.html
1) Same in as previous in executeProcess method.
2) getCommandLine (...)
+ /**
+ * @return The full command line for the ProcessBuilder.
+ */
+ public static String getCommandLine(ProcessBuilder pb) {
+ StringBuilder cmd = new StringBuilder();
+ for (String s : pb.command()) {
+ cmd.append(s).append(" ");
+ }
+ return cmd.toString();
+ }
Should we also trim cmd to remove last " "?
http://cr.openjdk.java.net/~miauno/6461635/webrev.01/jdk/test/lib/testlibrary/jdk/testlibrary/Utils.java.udiff.html
public static int waitForJvmPid(String key) throws Throwable {
Why it throw Trowable? Could we deal with exceptions in this method and
re-throw some more meaningful exception here?
Could you force flush for out here:
System.out.println("waitForJvmPid: Waiting for key '" + key + "'");
Hope it should be enough. It is scary to investigate such "timeouts".
Would it be better to add some
internal timeout in testlibrary? Really jtreg doesn't kill all processes
and we have alive java processes in such case.
For samevm tests timeouts are even worse. There is no good way to kill
test in samevm.
Should be
public static int tryFindJvmPid(String key) throws Throwable {
private? Are we going to use it in tests or only waitForJvmPid?
Leonid
>
> 5. Moved exit code check from the common utility function in ProcessThread.java to the test JstatdTest.java. The check is moved to the test because other tests in the future may have other expected exit codes.
>
>
> Webrev:
> http://cr.openjdk.java.net/~miauno/6461635/webrev.01/
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-6461635
>
> Mattias
>
>
> ----- Original Message -----
> From: mattias.tobiasson at oracle.com
> To: mikael.auno at oracle.com
> Cc: serviceability-dev at openjdk.java.net, Alan.Bateman at oracle.com
> Sent: Thursday, November 21, 2013 9:22:11 AM GMT +01:00 Amsterdam / Berlin / Bern / Rome / Stockholm / Vienna
> Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently
>
> I agree that merging both files in a single file may be good.
> The main reason why I did not do that is that I wanted to keep the history of the existing test. If I copy the test to a new file, all history of the code is lost.
>
> But this test bug was reported 8 years ago, and I don't believe much has changed in the code since then. So keeping the history may not be that important.
>
> Mattias
>
> ----- Original Message -----
> From: mikael.auno at oracle.com
> To: mattias.tobiasson at oracle.com, Alan.Bateman at oracle.com
> Cc: serviceability-dev at openjdk.java.net
> Sent: Wednesday, November 20, 2013 4:11:45 PM GMT +01:00 Amsterdam / Berlin / Bern / Rome / Stockholm / Vienna
> Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently
>
> How about defining the class that you want to attach to as a static
> inner class to the actual test? That would give you only one file, but
> with two classes, each with its own main method and clear correlation
> between them.
>
> Mikael
>
> On 2013-11-20 15:41, Mattias Tobiasson wrote:
>> Hi,
>> Each test requires 2 files, the actual test code and a helper file.
>>
>> The helper file will launch a separate java process (called Application), and then start the actual test. The actual test will then attach to the Application.
>>
>> For example:
>> PermissionTests.sh: Helper file that will launch Application instance and then start PermissionTest.java
>> PermissionTest.java: The actual test code that attaches to the Application.
>>
>> It is the PermissionTests.sh that is started by jtreg (contains the "@test"-tag).
>>
>> When I port PermissionTest.sh to java I would get 2 files called PermissionTest.java, so some name change is required.
>>
>> I could have kept the old PermissionTest.java unchanged, but then I would need another name for the prevoius PermissionTest.sh. And I wanted a "clean" Test name for the file containing the "@test"-tag.
>>
>> I used these names:
>> TestPermission.java: Helper file.
>> TestPermissionImpl.java: Actual test code.
>>
>> I am still new to adding tests for open jdk. I am happy to change the names if they do not follow the naming convention.
>>
>> Mattias
>>
>>
>> ----- Original Message -----
>> From: Alan.Bateman at oracle.com
>> To: mattias.tobiasson at oracle.com
>> Cc: serviceability-dev at openjdk.java.net
>> Sent: Wednesday, November 20, 2013 2:50:52 PM GMT +01:00 Amsterdam / Berlin / Bern / Rome / Stockholm / Vienna
>> Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently
>>
>>
>> Out of curiosity, what is the reason for the rename? I ask because we
>> use Basic and similar names in many areas. Also anything in the test
>> tree should be a test.
>>
>> -Alan.
>>
>> On 20/11/2013 13:47, Mattias Tobiasson wrote:
>>> Hi,
>>> Could you please review this fix.
>>>
>>> Summary of changes:
>>>
>>> 1. The real test bug fix is to add flag "-Xshare:off" when starting the "Application" instance. Without that flag, the test for ClassFileTransformer in RedefineAgent.java fails intermittently. The flag is added in function startApplication() in RunnerUtil.java.
>>>
>>> 2. Ported the following bash scripts to java:
>>> BasicTests.sh -> TestBasic.java
>>> PermissionTests.sh -> TestPermission.java
>>> ProviderTests.sh -> TestProvider.java
>>>
>>> 3. Renamed the java test code to avoid name clash with new java classes ported from bash script:
>>> BasicsTest.java -> TestBasicImpl.java
>>> PermissionTest.java -> TestPermissionImpl.java
>>> ProviderTest.java -> TestProviderImpl.java
>>>
>>> 4. Added some utility functions to jdk/testlibrary.
>>>
>>> 5. Moved exit code check from the common utility function in ProcessThread.java to the test JstatdTest.java. The check is moved to the test because other tests in the future may have other expected exit codes.
>>>
>>>
>>> Thanks,
>>> Mattias
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~miauno/6461635/webrev.00/
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-6461635
--
Leonid Mesnik
JVM SQE
More information about the serviceability-dev
mailing list