Hi Daniel, On Tue, 2019-02-26 at 16:50 +0000, Daniel Fuchs wrote: > Hi Severin, > > On 25/02/2019 15:31, Severin Gehwolf wrote: > > Hi Daniel, > > > > Thanks for the review! > > > > Latest webrev: > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8219585/02/webrev/ > > > [...] > > I'm not sure we need a larger timeout. It's 20 seconds on my machine > > which seems more than enough. What makes you think we'd need to get > > this increased and it would actually do anything for test stability? > > Meanwhile I launched your proposed changes through our > test system. The good news is that there was no failure > (even when repeating the test 25 times), and without > changing the 5 timeout - so that may be OK. > Maybe my machine has something that slows it down. > Or Maybe I should have used a timeout factor of 4. > It does pass if I use -timeout:4 when invoking it with jtreg.
FWIW, I use 'make test TEST="..."'. Isn't the testing system using that too? > The not so good news is that it seems none of our test > machine has a non-loopback address configured for local host. > So even with your changes, the test always trivially pass. > > I see the message says "Ignoring manual test" - but the > test is not manual? Like you've determined, it's manual. In the sense that it needs specific config to /etc/hosts on some test machine as described in the comment of the test. It needs to know some, non-loopback, IP address it can bind to for the purpose of the test. Then the test will use that address + loopback. That's the minimal test setup. Pre-patch, even such a setup was not sufficient. That's what this patch fixes. When I was working on that patch years ago, I think one testing machine got configured in a way so that it wouldn't trivially pass. That may have changed at some point in time? > [...] > > > Yes. I've tried to avoid those issues by allowing one non-loopback > > address and 127.0.0.1 explicitly. Is that a reasonable assumption? > > > > WRT using the loopback as an alternate configuration, > I wonder - should we add the loopback address before > checking size() < 1? > Not sure how stable the test will be in that case. It wouldn't help anything. In that case it wouldn't be a regression test anymore as it would pass *prior* JDK-6425769 *and* after. With a non-loopback address added too, it would fail prior and pass after. > Also should we use InetAddress.getLoopbackAddress() instead > of using "127.0.0.1" directly? InetAddress.getLoopbackAddress() is less deterministic than using IPv4 address. Interestingly the javadoc mentions it may return any loopback address, not just 127.0.0.1 or ::1. Not sure whether that's a problem or not, I'd be fine to change that if you prefer. Thanks, Severin
