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

[David Holmes]

On 24/05/2020 3:06 am, Chris Plummer wrote:
> 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.

Which is really an anti-pattern for this style of API :)

> 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();

Note the '.' should line up

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

All a matter of personal preference. :)

Cheers,
David

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