Hi Alex, Please review the updated webrev that has new test moved to test/jdk/comsun/jdi/connect folder.
Webrev: http://cr.openjdk.java.net/~dtitov/8163083/webrev.05/ Bug: https://bugs.openjdk.java.net/browse/JDK-8163083 Thanks! --Daniil On 9/24/18, 2:56 PM, "Alex Menkov" <alexey.men...@oracle.com> wrote: Daniil, Just remembered that SQE requested to not add new tests to vmTestbase (see test/hotspot/jtreg/vmTestbase/README.md) Could you please move the test to correct location (I suppose it's test/jdk/com/sun/jdi) --alex On 09/24/2018 14:30, Alex Menkov wrote: > +1 > > --alex > > On 09/24/2018 10:39, Gary Adams wrote: >> Looks good to me. >> >> On 9/24/18, 1:25 PM, Daniil Titov wrote: >>> Hi Alex and Gary, >>> >>> Please review the updated patch that includes suggested changes. >>> >>> http://cr.openjdk.java.net/~dtitov/8163083/webrev.04/ >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8163083 >>> >>> Thanks! >>> --Daniil >>> >>> >>> >>> >>> On 9/21/18, 3:59 PM, "Alex Menkov"<alexey.men...@oracle.com> wrote: >>> >>> One more note: >>> please add "@Override" annotation to the >>> SocketListeningConnector.updateArgumentMapIfRequired >>> >>> --alex >>> >>> On 09/21/2018 02:26, gary.ad...@oracle.com wrote: >>> > Looks good to me. >>> > >>> > For the javadoc >>> > >>> > 72 *<p> >>> > 73 * If<code>arguments</code> contains addressing >>> information. and >>> > 74 * only one connection will be accepted, the {@link >>> #accept accept} method >>> > 75 * can be called immediately without calling this >>> method. >>> > 76 * >>> > 77 * If the addressing information provided >>> in<code>arguments</code> >>> > implies >>> > 78 * the auto detection this information might be updated >>> with the address >>> > 79 * of the started listener. >>> > >>> > - you could add a<p> tag if you want to maintain the >>> spacing in the >>> > generated javadoc. >>> > - I've seen other changesets migrating to {@code ..} from >>> the older >>> > <code>...</code> >>> > >>> > It would be good to include some negative testing. >>> > Not sure if it is already covered in other tests, e.g. >>> > >>> > args1 = defaultArguments() >>> > startListening(args1) // bound port updated >>> > startListening(args1) // already listening >>> > >>> > >>> > On 9/20/18 10:59 PM, Daniil Titov wrote: >>> >> Please review the change that fixes the issue in >>> com.sun.tools.jdi.SocketListenerConnector.startListening() method. >>> >> >>> >> When the argument map passed to startListening() methods has >>> the port number unspecified or set to zero the port is auto detected. >>> However, the consequent call of startListening() methods with >>> unspecified port number fails rather than starts a new listener on >>> auto detected port. This happens since the original argument map >>> passed to the startListening() methods is used as a key to store the >>> mapping to the started listeners. >>> >> >>> >> The fix ensures that in cases when the port is auto detected >>> the argument map is updated with the bound port number. >>> >> >>> >> Mach5 vmTestbase_nsk_jdi tests successfully passed. >>> >> >>> >> Bug:https://bugs.openjdk.java.net/browse/JDK-8163083 >>> >> Webrev:http://cr.openjdk.java.net/~dtitov/8163083/webrev.02/ >>> >> >>> >> Thanks! >>> >> --Daniil >>> >> >>> >> >>> >> >>> > >>> >>> >>> >>
