RFR: 8146435: [TESTBUG] Logging tests are failing intermittently on windows when executed by JPRT

Rachel Protacio rachel.protacio at oracle.com
Wed Jan 20 16:53:32 UTC 2016


http://cr.openjdk.java.net/~rprotacio/8146435.03/

I went ahead and corrected the style in all the UL test files. If 
someone can review them to make sure I understood correctly, I'll go 
ahead and commit!

Thanks,
Rachel

On 1/20/2016 11:27 AM, Rachel Protacio wrote:
> Thanks for the reviews, Ioi and David! I'll fix those style issues.
> Rachel
>
> On 1/19/2016 11:45 PM, David Holmes wrote:
>> Hi Rachel,
>>
>> On 20/01/2016 3:51 AM, Rachel Protacio wrote:
>>> Hello, again!
>>>
>>> A call for brand new reviews; I've corrected the code as requested by
>>> Ioi, i.e. none of the tests use "java -version" at all and the
>>> ItablesTest forces the use of vtables, even when class sharing is
>>> enabled (as on windows). Thanks to Coleen for guiding me through the
>>> latter.
>>>
>>> The fixes pass JPRT once and for all! Webrev at
>>> http://cr.openjdk.java.net/~rprotacio/8146435.02/
>>
>> Functionally this all looks fine.
>>
>> test/runtime/logging/ClassInitializationTest.java (and elsewhere)
>>
>> Style Nit: The breakups of long processBuilder lines don't follow the 
>> Java style guide [1] for doing such changes. My preferred approach 
>> (Variant 3 of 'continuation lines') is to always keep the first 
>> argument to a call on the same line, then indent subsequent arguments 
>> to it on separate lines eg (I hope this formats correctly in email):
>>
>> ProcessBuilder pb = 
>> ProcessTools.createJavaProcessBuilder("-Xlog:classinit=info", 
>> "-Xverify:all", "-Xmx64m", "BadMap50");
>>
>> becomes:
>>
>> ProcessBuilder pb = 
>> ProcessTools.createJavaProcessBuilder("-Xlog:classinit=info",
>> "-Xverify:all",
>> "-Xmx64m",
>> "BadMap50");
>>
>> If that is still too long you can also break after the = and only 
>> indent the initial call line by 8 (Variant 1) then use variant 3 eg:
>>
>> ProcessBuilder pb =
>> ProcessTools.createJavaProcessBuilder("-Xlog:classinit=info",
>>                                               "-Xverify:all",
>>                                               "-Xmx64m",
>>                                               "BadMap50");
>>
>> [1] http://cr.openjdk.java.net/~alundblad/styleguide/ (did this get 
>> published yet?)
>>
>> Thanks,
>> David
>>
>>> I changed the copyrights in ClassB.java and VtablesTest.java because I
>>> had erroneously written them as 2015 when they weren't actually checked
>>> in until 2016.
>>>
>>> Thank you,
>>> Rachel
>>>
>>>
>>> On 1/13/2016 2:26 PM, Ioi Lam wrote:
>>>>
>>>>
>>>> On 1/13/16 7:32 AM, Rachel Protacio wrote:
>>>>> Hi,
>>>>>
>>>>> Yes, we have agreed not to use "-version" for triggering potentially
>>>>> changeable processes. Ioi, I think you may have misread my code as I
>>>>> did not use "java -version" for ItablesTest.java.
>>>> Hi Rachel,
>>>>
>>>> Thanks for the explanation. I looked at ItablesTest again. It uses
>>>> ClassB, which is pretty simple and by itself won't generate the
>>>> "vtable index" output. If it's not too much effort, instead of
>>>> removing the that output from the test, I would suggest changing
>>>> ClassB so that it would produce the desired output.
>>>>
>>>> Thanks
>>>>
>>>> - Ioi
>>>>
>>>>
>>>>> I had left it in the DefaultMethodsTest.java and
>>>>> ClassInitializationTest.java tests because they are testing for
>>>>> output that seemed to be independent of -version implementation, in
>>>>> particular, java/lang/String default methods and any initialization
>>>>> that had no class initialization side effects (such as
>>>>> java.io.OutputStream), respectively. But I can make them more robust
>>>>> by running on a dummy class.
>>>>>
>>>>> As for the CDS, I realize I misinterpreted your conclusion. I can get
>>>>> rid of "-Xshare:off" and remove the parts of the tests looking for
>>>>> lines that disappear when CDS is enabled.
>>>>>
>>>>> Thanks,
>>>>> Rachel
>>>>>
>>>>> On 1/12/2016 9:45 PM, David Holmes wrote:
>>>>>> I agree with Ioi. This is similar to the monitor inflation/deflation
>>>>>> logging test - we need to ensure we use something guaranteed to
>>>>>> always generate the expected log output.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>> On 13/01/2016 11:50 AM, Ioi Lam wrote:
>>>>>>> Rachel,
>>>>>>>
>>>>>>> I don't think adding "-Xshare:off" is the correct fix. For example,
>>>>>>> ItablesTest.java just runs "java -version", and depends on the fact
>>>>>>> that
>>>>>>> at least one of the JDK classes that are implicitly loaded has an
>>>>>>> itable
>>>>>>> (or whatever contents that produces the "vtable index " output).
>>>>>>>
>>>>>>> This behavior depends on internal JDK implementation and there's no
>>>>>>> guarantee that it will always be true. There's also no guarantee 
>>>>>>> that
>>>>>>> the behavior will be the same across all the OS ports.
>>>>>>>
>>>>>>> In the future, some changes of "java -version" may cause this 
>>>>>>> test to
>>>>>>> fail only on a particular platform. I don't want someone to spend a
>>>>>>> day
>>>>>>> to track down some imaginative windows issues again just to find 
>>>>>>> out
>>>>>>> that it's a test bug.
>>>>>>>
>>>>>>> So, as I mentioned in the bug report, the tests should be 
>>>>>>> written to
>>>>>>> load a class that *you have written* to ensure that the output 
>>>>>>> will be
>>>>>>> there regardless of JDK implementation.
>>>>>>>
>>>>>>> Here are other cases where the tests are dependent on unspecified
>>>>>>> behaviors:
>>>>>>>
>>>>>>> out.shouldContain("[Initialized").shouldContain("without side
>>>>>>> effects]");
>>>>>>>
>>>>>>>       All of the output in DefaultMethodsTest.java
>>>>>>>
>>>>>>> In fact, I think you should remove "-Xshare:off" from the tests.
>>>>>>> Adding
>>>>>>> -Xshare:off will also force the tests to be executed with CDS
>>>>>>> disabled,
>>>>>>> and would cover up problems that only show up when CDS is enabled.
>>>>>>>
>>>>>>> Thanks
>>>>>>> - Ioi
>>>>>>>
>>>>>>> On 1/12/16 10:27 AM, Rachel Protacio wrote:
>>>>>>>> Hello!
>>>>>>>>
>>>>>>>> Please review this fix which adds "-Xshare:off" to all UL tests'
>>>>>>>> ProcessBuilders to prevent windows failures. Extreme thanks to Ioi
>>>>>>>> for
>>>>>>>> debugging and finding the source of the problem.
>>>>>>>>
>>>>>>>> Open webrev: http://cr.openjdk.java.net/~rprotacio/8146435/
>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8146435
>>>>>>>>
>>>>>>>> Tests sent to JPRT, pass on all platforms.
>>>>>>>>
>>>>>>>> Thank you!
>>>>>>>> Rachel
>>>>>>>
>>>>>
>>>>
>>>
>



More information about the hotspot-runtime-dev mailing list