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

Wed Jan 20 16:27:46 UTC 2016

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
>>>>>>
>>>>
>>>
>>