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

[David Holmes]

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