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

[David Holmes]

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

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