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

Rachel Protacio rachel.protacio at oracle.com
Wed Jan 20 17:39:48 UTC 2016 

Thank you, Max!


On 1/20/2016 12:29 PM, Max Ockner wrote:
> Looks good to me.
> Thanks,
> Max
> On 1/20/2016 11:53 AM, Rachel Protacio wrote:
>> 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