RFR (S): 8230854: Cleanup SuppressWarnings in test lib and remove noisy traces in StreamPumper

[David Holmes]

Hi Christoph,

On 12/09/2019 5:23 pm, Langer, Christoph wrote:
> Hi David,
> 
>> test/lib/jdk/test/lib/Platform.java
>> test/lib/jdk/test/lib/process/ProcessTools.java
>>
>> in this code
>>
>>                 } catch (PrivilegedActionException e) {
>> -                 @SuppressWarnings("unchecked")
>>                     IOException t = (IOException) e.getException();
>>
>> I take it these are unnecessary as these casts are not unchecked casts?
> 
> Yes, unchecked casts would rather occur with generics. This plain type cast will be checked at runtime. So, no javac or IDE warning if @SuppressWarnings("unchecked") is removed here.
> 
>> test/lib/jdk/test/lib/process/ProcessTools.java
>>
>> -     @SuppressWarnings("overloads")
>>         public static Process startProcess(String name,
>>                                            ProcessBuilder processBuilder,
>>                                            final Predicate<String>
>> linePredicate)
>>
>> Unnecessary? I can't find what this warning is supposed to mean. Is it used by some IDE's?
> I didn't find any reference either and the IDE says that this is an unknown annotation. So unless somebody steps up and tells us what it's good for, I guess it can be removed.

Not it is a valid warning. Try compiling with -Xlint:all without the 
warning suppressed:

----------System.err:(19/1295)----------
/scratch/users/daholme/jdk-dev2/open/test/lib/jdk/test/lib/process/ProcessTools.java:91: 
warning: [overloads] 
startProcess(String,ProcessBuilder,Consumer<String>) in ProcessTools is 
potentially ambiguous with 
startProcess(String,ProcessBuilder,Predicate<String>) in ProcessTools
     public static Process startProcess(String name,
                           ^
Thanks,
David
-----

>>> Furthermore, when analyzing some test outputs lately, I got annoyed by
>> stack traces like the following when using
>> jdk.test.lib.process.StreamPumper. I think they are of no particular value and
>> could be suppressed.
>>>
>>> java.io.IOException: Stream closed
>>> at
>> java.base/java.io.BufferedInputStream.getBufIfOpen(BufferedInputStream.
>> java:169)
>>> at
>> java.base/java.io.BufferedInputStream.read(BufferedInputStream.java:335)
>>> at
>> java.base/java.io.BufferedInputStream.fill(BufferedInputStream.java:245)
>>> at
>> java.base/java.io.BufferedInputStream.read1(BufferedInputStream.java:285
>> )
>>> at
>> java.base/java.io.BufferedInputStream.read(BufferedInputStream.java:344)
>>> at java.base/java.io.FilterInputStream.read(FilterInputStream.java:107)
>>> at jdk.test.lib.process.StreamPumper.run(StreamPumper.java:109)
>>> at
>> java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.ja
>> va:515)
>>> at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
>>> at java.base/java.lang.Thread.run(Thread.java:830)
>>
>> Ok. Seems a bit crude, but then the StreamPumper code is crude - just
>> running till it hits an exception, but an exception is the only way it
>> will detect an async close of the stream.
> 
> Yep.
> 
> So, thanks for the review. Nightly test suite didn't show regressions. I guess I'll wait a bit until I push it if somebody still raises concerns.
> 
> Best regards
> Christoph
>