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

[Daniil Titov]

Hi Chris and David,

Please review a new version of the fix [1] with the changes Chris suggested.

[1] Webrev:  http://cr.openjdk.java.net/~dtitov/8244993/webrev.02
[2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8244993 

Thank you,
Daniil

On 5/22/20, 11:50 AM, "Chris Plummer" <chris.plummer at oracle.com> 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. 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
    >
    >