RFR: 8196750 [Testbug] tools/launcher tests need to tolerate unrelated warnings
Hi, Please review fix in launcher tests. Review: http://cr.openjdk.java.net/~anazarov/JDK-8196750/webrev/ Bug: https://bugs.openjdk.java.net/browse/JDK-8196750?filter=-1 —Thanks, Andrei
Re: RFR: 8196750 [Testbug] tools/launcher tests need to tolerate unrelated warnings
Hi Andrei, On 22/03/2018 11:12 AM, Andrey Nazarov wrote: Hi, Please review fix in launcher tests. Review: http://cr.openjdk.java.net/~anazarov/JDK-8196750/webrev/ Bug: https://bugs.openjdk.java.net/browse/JDK-8196750?filter=-1 test/jdk/tools/launcher/ToolsOpts.java I don't understand how the change fixes the problem. IIUC the problem is that the test expects the output to consist of the command-line passed to the tool. Instead if it contains a Warning from the VM, it won't match and so we fail. The new code no longer uses a String[] and no longer assumes that output[i] == args[i], but it still searches the current "output" value for a match with one of the args. But that will still fail if what we actually have is a Warning. ?? I would have expected to see the Warning strings filtered out more directly. The other test change seems reasonable. Thanks, David —Thanks, Andrei
Re: RFR: 8196750 [Testbug] tools/launcher tests need to tolerate unrelated warnings
On 22/03/2018 12:52 PM, Andrei Nazarov wrote: On Mar 21, 2018 19:13, David Holmes wrote: Hi Andrei, On 22/03/2018 11:12 AM, Andrey Nazarov wrote: > Hi, > > Please review fix in launcher tests. > > Review: http://cr.openjdk.java.net/~anazarov/JDK-8196750/webrev/ > Bug: https://bugs.openjdk.java.net/browse/JDK-8196750?filter=-1 test/jdk/tools/launcher/ToolsOpts.java I don't understand how the change fixes the problem. IIUC the problem is that the test expects the output to consist of the command-line passed to the tool. Instead if it contains a Warning from the VM, it won't match and so we fail. The new code no longer uses a String[] and no longer assumes that output[i] == args[i], output now is List and output.get(j).equals(arg[j]) has the same sematics. The difference is that it *searches* args as a line in the output, so it would skip any Warnings lines. So for each arg it searches the entire output. Okay I see now. Thanks, David - Output is scanned once so it checks order of args as well. -- Andrei it still searches the current "output" value for a match with one of the args. But that will still fail if what we actually have is a Warning. ?? I would have expected to see the Warning strings filtered out more directly. The other test change seems reasonable. Thanks, David > —Thanks, > Andrei >
Re: RFR: 8196750 [Testbug] tools/launcher tests need to tolerate unrelated warnings
David, Why would the VM emit these warnings if the deprecated vm flag is not being used ? Andrey, As for the reviews I am ok with InfoStreams, wrt. ToolOpts it is not at all apparent, maybe more comments explaining what is going on ? Kumar Hi Andrei, On 22/03/2018 11:12 AM, Andrey Nazarov wrote: Hi, Please review fix in launcher tests. Review: http://cr.openjdk.java.net/~anazarov/JDK-8196750/webrev/ Bug: https://bugs.openjdk.java.net/browse/JDK-8196750?filter=-1 test/jdk/tools/launcher/ToolsOpts.java I don't understand how the change fixes the problem. IIUC the problem is that the test expects the output to consist of the command-line passed to the tool. Instead if it contains a Warning from the VM, it won't match and so we fail. The new code no longer uses a String[] and no longer assumes that output[i] == args[i], but it still searches the current "output" value for a match with one of the args. But that will still fail if what we actually have is a Warning. ?? I would have expected to see the Warning strings filtered out more directly. The other test change seems reasonable. Thanks, David —Thanks, Andrei
Re: RFR: 8196750 [Testbug] tools/launcher tests need to tolerate unrelated warnings
On 23/03/2018 12:18 AM, Kumar Srinivasan wrote: David, Why would the VM emit these warnings if the deprecated vm flag is not being used ? The warnings are not deprecation warnings. The warnings are for flags that should have been made obsolete in this version but are still present. It is a reminder to make updates when the version ticks over such that a deprecated flag should now be obsolete or expired. But more generally the VM can issue runtime warnings for a variety of environmental issues. David Andrey, As for the reviews I am ok with InfoStreams, wrt. ToolOpts it is not at all apparent, maybe more comments explaining what is going on ? Kumar Hi Andrei, On 22/03/2018 11:12 AM, Andrey Nazarov wrote: Hi, Please review fix in launcher tests. Review: http://cr.openjdk.java.net/~anazarov/JDK-8196750/webrev/ Bug: https://bugs.openjdk.java.net/browse/JDK-8196750?filter=-1 test/jdk/tools/launcher/ToolsOpts.java I don't understand how the change fixes the problem. IIUC the problem is that the test expects the output to consist of the command-line passed to the tool. Instead if it contains a Warning from the VM, it won't match and so we fail. The new code no longer uses a String[] and no longer assumes that output[i] == args[i], but it still searches the current "output" value for a match with one of the args. But that will still fail if what we actually have is a Warning. ?? I would have expected to see the Warning strings filtered out more directly. The other test change seems reasonable. Thanks, David —Thanks, Andrei
Re: RFR: 8196750 [Testbug] tools/launcher tests need to tolerate unrelated warnings
> On 22 Mar 2018, at 07:18, Kumar Srinivasan > wrote: > > David, > > Why would the VM emit these warnings if the deprecated vm flag > is not being used ? > > Andrey, > As for the reviews I am ok with InfoStreams, wrt. ToolOpts it is not at all > apparent, > maybe more comments explaining what is going on ? Added more comments. see http://cr.openjdk.java.net/~anazarov/JDK-8196750/webrev.02/ > > Kumar > > >> Hi Andrei, >> >> On 22/03/2018 11:12 AM, Andrey Nazarov wrote: >>> Hi, >>> >>> Please review fix in launcher tests. >>> >>> Review: http://cr.openjdk.java.net/~anazarov/JDK-8196750/webrev/ >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8196750?filter=-1 >> >> test/jdk/tools/launcher/ToolsOpts.java >> >> I don't understand how the change fixes the problem. IIUC the problem is >> that the test expects the output to consist of the command-line passed to >> the tool. Instead if it contains a Warning from the VM, it won't match and >> so we fail. The new code no longer uses a String[] and no longer assumes >> that output[i] == args[i], but it still searches the current "output" value >> for a match with one of the args. But that will still fail if what we >> actually have is a Warning. ?? I would have expected to see the Warning >> strings filtered out more directly. >> >> The other test change seems reasonable. >> >> Thanks, >> David >> >>> —Thanks, >>> Andrei >>> >
Re: RFR: 8196750 [Testbug] tools/launcher tests need to tolerate unrelated warnings
Looks good, thanks for doing this!. Kumar On 4/4/2018 5:09 PM, Andrey Nazarov wrote: On 22 Mar 2018, at 07:18, Kumar Srinivasan wrote: David, Why would the VM emit these warnings if the deprecated vm flag is not being used ? Andrey, As for the reviews I am ok with InfoStreams, wrt. ToolOpts it is not at all apparent, maybe more comments explaining what is going on ? Added more comments. see http://cr.openjdk.java.net/~anazarov/JDK-8196750/webrev.02/ Kumar Hi Andrei, On 22/03/2018 11:12 AM, Andrey Nazarov wrote: Hi, Please review fix in launcher tests. Review: http://cr.openjdk.java.net/~anazarov/JDK-8196750/webrev/ Bug: https://bugs.openjdk.java.net/browse/JDK-8196750?filter=-1 test/jdk/tools/launcher/ToolsOpts.java I don't understand how the change fixes the problem. IIUC the problem is that the test expects the output to consist of the command-line passed to the tool. Instead if it contains a Warning from the VM, it won't match and so we fail. The new code no longer uses a String[] and no longer assumes that output[i] == args[i], but it still searches the current "output" value for a match with one of the args. But that will still fail if what we actually have is a Warning. ?? I would have expected to see the Warning strings filtered out more directly. The other test change seems reasonable. Thanks, David —Thanks, Andrei
