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

[David Holmes]

On 21/01/2016 2: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!

Looks good!

Thanks,
David

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