RFR JDK-8196748,tools/jar tests need to tolerate unrelated warnings

[David Holmes]

Hi Sherman,

On 13/03/2018 5:13 AM, Xueming Shen wrote:
> On 3/11/18, 7:48 PM, David Holmes wrote:
>> Hi Sherman,
>>
>>
>> Thanks for working on this and adding the new functionality to 
>> OutputAnalyzer, but note that:
>>
>> +   private static final String jvmwarningmsg =
>> +       "Java HotSpot\\(TM\\) 64-Bit Server VM warning:.*";
>>
>> is Oracle JDK specific and 64-bit and server VM specific! Other tests 
>> use:
>>
>> Pattern.compile(".*VM warning.*")
>>
>> to exclude all VM warnings. Sorry that wasn't clear from previous 
>> discussions.
> 
> Hi David,
> 
> I was trying to be conservative to only filter out the "vm warnings" 
> particularly for this
> test failure. The webrev has been updated as suggested to filter out any 
> line with
> "VM warning".

The issue isn't about being conservative. You need to exclude the 
warnings no matter what VM the test is executed on. The original code 
would only pass on the Oracle JDK, and 64-bit Server VM. If run on 
OpenJDK the warning has the form:

OpenJDK 64-Bit Server VM warning: ...

If run on 32-bit there's no "64-bit". If run on Client VM there's no 
"Server".

>>
>>> Verified with the warning msg turned on as
>>> http://cr.openjdk.java.net/~sherman/8196748/webrev.hs/src/hotspot/share/runtime/arguments.cpp.sdiff.html 
>>>
>>> and ran the jtreg with -XX:+FastTLABRefill.
>>
>> That will not hit all cases as it requires the -XX:+FastTLABRefill to 
>> be passed through to all JVMs launched by tests. The original problem 
>> occurred with unconditional warnings that did not depend on a 
>> particular flag being specified on the command-line. All of those 
>> cases have now been fixed however so just reenabling the message 
>> doesn't achieve anything.
> 
> The reason I did not try to turn "PrintSafepointStatisticsCount" back on 
> (to reproduce) is that it was
> not included in the changeset published in JDK-8196739 as
> http://hg.openjdk.java.net/jdk/jdk/rev/74be5b4ed152
> 
> It appears the reason I failed to reproduce LeadingGarbage.java is the 
> test in my repo does not forward
> the vm options (I'm not sure why the test forwards those vm options in 
> your repo). I have updated the test

I think there is a misunderstanding here. It doesn't matter what flag 
generates the warning - I modified PrintSafepointStatisticsCount as a 
convenience because the flags that originally triggered this problem no 
longer exist as flags (and so can't trigger the problem). The flag that 
generates the warning does not get specified as a VM argument to jtreg 
and does not get passed through to any other tools or VMs launched 
(there's no difference between our repos). That is the key point. In its 
original form (before the relevant flags were actually removed) we 
experience unconditional warnings from the VM and every execution of the 
VM (whether java, javac, jar, javap etc) is affected. Your test only 
triggered a warning due to the use of the FastTLABRefill flag and only 
the VM's that are explicitly passed that flag will encounter the 
warning. That excluded the VM (jar?) that actually triggers the test 
failure.

> case to use the new test.lib (it was using the "old" jdk.testlibrary), 
> and forward the vmoptions (this one
> should have nothing to do with old libs or new libs though)

Right. Good to modernize the test, but not directly related to current 
issue.

> Now the failure is reproducible and has been fixed with new method in 
> OutputAnalyzer.java.
> 
> webrev: http://cr.openjdk.java.net/~sherman/8196748/webrev

test/jdk/tools/jar/modularJar/Basic.java

+                     } else if (line.contains("Java HotSpot(TM) 64-Bit 
Server VM warning:")) {
+                         continue;  // ignore server vm warning see#8196748

That needs to be any VM warning as discussed.

---

test/lib/process/OutputAnalyzer.java

I'm not sure if "replace" is the right strategy here, if stderr may be 
very large. ?? The approach in JDK-8194067 was to see if the output 
lines matched the warning (though there are a couple of deficiencies in 
the way it did it).

Otherwise, my only minor concern is the assumption that the VM warnings 
will always be on stderr. That is the default, but it can be changed.

Thanks,
David

> (arguments.cpp will not be put back, just to help reproduce)
> 
> Thanks,
> Sherman