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
