RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings

[Chris Plummer]

On 5/23/20 6:03 AM, David Holmes wrote:
> Hi Chris,
>
> On 23/05/2020 4:50 am, Chris Plummer wrote:
>> Hi Daniil,
>>
>> There is one reference to "jvmwarningmsg" that occurs before it is 
>> declared while all the rest all come after. It probably would make 
>> sense to move its declaration up near the top of the file.
>>
>>    92     private static void matchListedProcesses(OutputAnalyzer 
>> output) {
>>    93         output.stdoutShouldMatchByLine(JCMD_LIST_REGEX)
>>    94                 .stderrShouldBeEmptyIgnoreVMWarnings();
>>    95     }
>>
>> We probably use this coding pattern all over the place, but I think 
>> it just leads to less readable code. Any reason not to use:
>>
>>    92     private static void matchListedProcesses(OutputAnalyzer 
>> output) {
>>    93         output.stdoutShouldMatchByLine(JCMD_LIST_REGEX);
>>    94         output.stderrShouldBeEmptyIgnoreVMWarnings();
>>    95     }
>>
>> I just don't see the point of the chaining, and don't understand why 
>> all these OutputAnalyzer methods return the "this" object in the 
>> first place. Maybe I'm missing something.
>
> They return "this" precisely so that you can chain. The API was 
> designed for a style that allows:
>
> output.shouldContain(x).shouldNotContain(y).shouldContain(z) ...
>
> to avoid the repetition of "output".
Yeah, I get that, but I never did like this pattern. I just don't find 
it as readable. For one, there's no conveyance of the method return 
type, not just because of the chaining, but also because the method name 
does not imply a return type. Chaining like 
getMethod().getClass().getName() is fine, because there are implied 
return types in the method names, and they clearly are being called for 
the purpose of returning a type. But when the return type is there 
solely for the purpose of chaining, it's not as obvious what is going on.

Your example is easier to read because the method names are short, 
readily identified as related, and you made them all fit on one line 
with shortened arguments. That's not the case with Daniil's code. I just 
don't see the argument for saying that:

    93         output.stdoutShouldMatchByLine(JCMD_LIST_REGEX)
    94                 .stderrShouldBeEmptyIgnoreVMWarnings();

Is somehow better than:

    93         output.stdoutShouldMatchByLine(JCMD_LIST_REGEX);
    94         output.stderrShouldBeEmptyIgnoreVMWarnings();

I don't have to look twice at the second version (or be familiar with 
the APIs being used) to know what's going on.

Chris
>
> David
> -----
>
>  I guess maybe there are cases where
>> the OutputAnalyzer object is not already in a local variable, adding 
>> some value to the chaining, but that's not the case here, and I think 
>> if it were the case it would be more readable just to stick the 
>> OutputAnalyzer object in a local. There one other case of this:
>>
>>   154     private static void matchPerfCounters(OutputAnalyzer output) {
>>   155 output.stdoutShouldMatchByLine(PERF_COUNTER_REGEX,null,
>>   156                 PERF_COUNTER_REGEX)
>>   157                 .stderrShouldBeEmptyIgnoreVMWarnings();
>>   158     }
>>
>> I think you can also add spaces after the commas, and probably make 
>> the first stdoutShouldMatchByLine() one line.
>>
>> thanks,
>>
>> Chris
>>
>> On 5/21/20 10:06 PM, Daniil Titov wrote:
>>> Please review a webrev [1] that reverts the changes done in 
>>> jdk.test.lib.process.OutputAnalyzer in [3].
>>>
>>> Change [3] modified OutputAnalyzer 
>>> stderrShouldBeEmptyIgnoreVMWarnings() methods to ignore also VM 
>>> version strings . The current webrev [1] reverts this change and 
>>> instead makes the tests that expects an empty stderr from launched 
>>> j-* tools to filter out '-showversion' from test options when 
>>> forwarding test VM option to j*-tools.
>>>
>>> Testing: Mach5 tier1-tier5 tests passed successfully. Tier6-tier7 
>>> tests are in progress.
>>>
>>> [1]  Webrev: http://cr.openjdk.java.net/~dtitov/8244993/webrev.01
>>> [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8244993
>>> [3] https://bugs.openjdk.java.net/browse/JDK-8242009
>>>
>>> Thank you,
>>> Daniil
>>>
>>>
>>
>>