Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
Hi Chris and David, Thank you for reviewing this change. --Best regards, Daniil On 5/26/20, 4:33 PM, "Chris Plummer" 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" 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 > > > > > > > >
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
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" 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 > >
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
Hi Chris, I like the pattern and use it quite often. And the main reason not to avoid repetition of the object, but to avoid stupid copy-paste errors like MyObject obj1 = new MyObject(); obj1.method1(); obj1.method2(); MyObject obj2 = new MyObject(); obj2.method1(); obj1.method2(); <== I fixed a lot of such error With this pattern you just do MyObject obj1 = new MyObject() .method1() .method2(); MyObject obj2 = new MyObject() .method1() .method2(); Just my 2 cents.. --alex On 05/23/2020 10:06, 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. 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(); 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. 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
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
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" 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 > >
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
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
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
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. 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(); 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. 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
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
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
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
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
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
Hi Daniil, On 22/05/2020 5:24 pm, Daniil Titov wrote: Hi David, Some tiers in Mach5 are configured to run tests with '-showversion' VM options. In JDK-8242009 [3] we started forwarding test VM options to j-*tools Okay. Filtering it out seems fine then. Thanks, David and some tests that launch them and expect an empty stderr (apart from VM warnings) need to be corrected to either ignore version messages in the output or to ensure that -showversion VM option is not passed to j*-tool even if it is passed to the test itself. Best regards, Daniil On 5/21/20, 10:41 PM, "David Holmes" wrote: Hi Dannil, On 22/05/2020 3: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 Thank you for reverting the OutputAnalyzer 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. That seems workable, though I'm unclear what is adding the "-showversion" in the first place? Thanks, David - > 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 > >
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
Hi David, Some tiers in Mach5 are configured to run tests with '-showversion' VM options. In JDK-8242009 [3] we started forwarding test VM options to j-*tools and some tests that launch them and expect an empty stderr (apart from VM warnings) need to be corrected to either ignore version messages in the output or to ensure that -showversion VM option is not passed to j*-tool even if it is passed to the test itself. Best regards, Daniil On 5/21/20, 10:41 PM, "David Holmes" wrote: Hi Dannil, On 22/05/2020 3: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 Thank you for reverting the OutputAnalyzer 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. That seems workable, though I'm unclear what is adding the "-showversion" in the first place? Thanks, David - > 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 > >
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
Hi Dannil, On 22/05/2020 3: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 Thank you for reverting the OutputAnalyzer 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. That seems workable, though I'm unclear what is adding the "-showversion" in the first place? Thanks, David - 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
RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
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