RFR (XS) 8202360: [TESTBUG] runtime/LoadClass/TestResize.java needs to print output when it fails

[David Holmes]

Hi Gerard,

On 19/05/2018 4:54 AM, Gerard Ziemski wrote:
> hi David,
> 
>> On May 18, 2018, at 12:59 AM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> Hi Gerard,
>>
>> On 18/05/2018 2:21 AM, Gerard Ziemski wrote:
>>> hi David,
>>> Thank you for the review!
>>>> On May 16, 2018, at 4:54 PM, David Holmes <david.holmes at oracle.com> wrote:
>>>>
>>>> Hi Gerard,
>>>>
>>>> On 17/05/2018 3:47 AM, Gerard Ziemski wrote:
>>>>> Hi all,
>>>>> Please review this small enhancement where we print out additional information (i.e. output from PrintSystemDictionaryAtExit), when we detect the failure.
>>>>> https://bugs.openjdk.java.net/browse/JDK-8202360
>>>>> http://cr.openjdk.java.net/~gziemski/8202360_rev1/
>>>>
>>>> I don't understand what your fix is doing - sorry.
>>> The fix takes all the available output bytes from the process (using Scanner to grab all tokens at once) and prints all the output from the process (i.e. PrintSystemDictionaryAtExit, and there is a lot of it) at failure.
>>
>> Okay but that's the rest of the output that hasn't yet been processed - correct? So the line of output that caused the failure won't be there as it's already been grabbed by the scanner and processed.
> 
> Good catch, I changed the fix to make sure we print all the output.
> 
> 
>>>>
>>>> What I wanted to see was the complete history of lines of the form
>>>>
>>>> "Java dictionary (table_size=107, classes=6)"
>>>>
>>>> so that we can see how the table size and number of classes have changed leading up to the failure. But I realize now that we already get that from the:
>>>>
>>>> System.out.println("PASS table_size:"+table_size+", classes:"+classes+" OK");
>>>>
>>>> All that is missing is the printing of the table_size and classes when we throw the exception. That can be trivially fixed:
>>>>
>>>>          if (loadFactor > MAX_LOAD_FACTOR) {
>>>> +          System.out.println("FAIL table_size:"+table_size+", classes:"+classes);
>>>>            throw new RuntimeException("Load factor too high, expected MAX "+MAX_LOAD_FACTOR+", got "+loadFactor);
>>>>          } else {
>>>>            System.out.println("PASS table_size:"+table_size+", classes:"+classes+" OK");
>>>>          }
>>> OK, so you determined that only that one piece of info was missing in the end in your case, but maybe the next person might want to see everything that PrintSystemDictionaryAtExit provides?
>>> As long as we’re touching the test - should we then stick with the solution that prints out everything from PrintSystemDictionaryAtExit?
>>
>> I'm not sure how anything that comes later in PrintSystemDictionaryAtExit would help with a failure that happened earlier, but if you see value it then that's fine. However I think you need my suggestion as well to get the information that actually caused the failure.
> 
> We now print all the output from PrintSystemDictionaryAtExit, but 

So you'll print all the good reports for the "Java dictionary ..." lines 
then if there is a failure print the entire output. That's okay - though 
as I mention in the bug report it does tend to hit the jtreg output 
buffer limit.

> also following your suggestion we summarize the failure details in
> the exception itself like:
> 
> java.lang.RuntimeException: Load factor too high, expected MAX 5.0, got 467.30841121495325 [table size 107, number of clases 50002]

Okay - that helps with the immediate problem I had. I also now realize 
that the failure in this test is showing is that the SD did not in fact 
re-size as expected!

>> Some sample output in the bug report would be useful.
> 
> I attached a sample output to the issue as requested.

Thanks.

> 
> https://bugs.openjdk.java.net/browse/JDK-8202360
> http://cr.openjdk.java.net/~gziemski/8202360_rev2

A few suggestions:

   73     String string_out = "";
   74     while (line != null) {
   75       string_out += line+"\n";

As this is not in fact a constant I would suggest using a (large-size) 
StringBuilder directly and avoid all the implicit StringBuilder and 
String usage that the '+' operator will hide. The variable can just be 
called 'output'.

  88           System.out.println(string_out);

Please precede with a comment

// We've hit an error so print all of the output parsed so far, and
// then the remaining output.

  93           throw new RuntimeException("Load factor too high, 
expected MAX "+MAX_LOAD_FACTOR+", got "+loadFactor+" [table size 
"+table_size+", number of clases "+classes+"]");


Please split this line up to make it much shorter. I'm tempted to ask 
you to fix this missing spaces around the '+' operator but that 
bad-style is prevalent throughout this test. (This violates a number of 
Java style rules.)

Don't forget to add second copyright year.

Thanks,
David

> cheers
>