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

[Daniil Titov]

Hi Chris and David,

Thank you for reviewing this change.

--Best regards,
Daniil

On 5/26/20, 4:33 PM, "Chris Plummer" <chris.plum...@oracle.com> wrote:

    Hi Daniil,

    Looks good.

    thanks,

    Chris

    On 5/26/20 10:46 AM, Daniil Titov wrote:
    > 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.plum...@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
    >      >
    >      >
    >
    >
    >
    >