RFR: 8219585: [TESTBUG] sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java passes trivially when it shouldn't

[Severin Gehwolf]

Hi Daniel,

Thanks for the review!

Latest webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8219585/02/webrev/

On Fri, 2019-02-22 at 18:31 +0000, Daniel Fuchs wrote:
> Hi Severin,
> 
> Did you manage to make the test pass locally?

Yes. Here is the latest run:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8219585/02/JMXInterfaceBindingTest.jtr

FWIW, it passed locally with the v1 change proposed. But then again, it
didn't do what it was intended for: Try to bind to multiple host:port
pairs on a single system. This didn't work prior JDK-6425769 as it
would bind to *all* interfaces. The updated webrev tries to remediate
this.

> This test has had a long history of failing, and I've come
> to suspect that it might be flawed.

If it has a history of failing, why aren't there any bugs for it? How
did it fail? Are you referring to JDK-8145982 and JDK-8146015?

> First - it has /timeout=5. I believe that's much too short.
> It immediately failed the first time I ran it locally on my machine.

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?

> Then it uses well known ports. That's bad. There's no guarantee
> that these ports will be free.

Fair enough. Fixed in the latest webrev. Now picks a random port in
range [9100, 9200].

> Then - it only use addresses configured for "localhost".
> 
> Can there ever be more than one such address?

Yes, a multi-homed computer could use this for example this in
/etc/hosts on Linux:

192.168.1.18 localhost
192.168.122.1 localhost

Looks like this on my system:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8219585/02/JMXInterfaceBindingTest-multihome.jtr

> If there is more
> than one, I suspect the test will fail: I believe the subprocess
> started by the test would need the additional
>     -Djava.rmi.server.hostname=<adddress>

That's needed for 127.0.0.1 (loopback) support, so I've added it. Other
IP addresses don't seem to need it. So the test does not need it per
se, but it's useful so that more configs can actually test this.

> on the command line.
> And even with that - it might still fail if those two addresses
> can't be bound simultaneously on the same port.

No. Socket addresses are unique per host:port pair. If that wasn't
possible, the whole test would be pointless.

> Then why are loopback addresses filtered out? Because of two
> things I guess:
> 
>     - if several loopback are defined, I'm not sure you can
>       bind them on the same port (and it might be difficult to
>       figure that out)
> 
>     - if you bind to the loopback address, then you most probably
>       need to start the subprocess with
>        -Djava.rmi.server.hostname=<loopback-adddress>

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?

> If we don't see this test failing, I suspect this is because
> it is either never selected, or always passes trivially.
> If we start selecting it - or have a configuration where it
> doesn't trivially pass, I suspect it will fail.

My bet is on it passing trivially with:

"Ignoring manual test since no more than one IPs are configured for 'localhost'"

Then again, I don't know about Oracle's test selection settings.

Thanks,
Severin

> 
> 
> On 22/02/2019 15:04, Severin Gehwolf wrote:
> > Hi!
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8219585
> > 
> > Could somebody please review this trivial testbug. For a config like
> > [1] the logic prior JDK-8145982 returned this list for
> > getAddressesForLocalHost():
> > 
> > [localhost/127.0.0.1, localhost/192.168.1.18, localhost/0:0:0:0:0:0:0:1]
> > 
> > Post JDK-8145982, getAddressesForLocalHost() returns:
> > 
> > [localhost/192.168.1.18]
> > 
> > The fix is trivial. Just adjust the condition for as to when the test
> > should actually trivially pass:
> > 
> > diff --git a/test/jdk/sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java b/test/jdk/sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java
> > --- a/test/jdk/sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java
> > +++ b/test/jdk/sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java
> > @@ -176,8 +176,8 @@
> >       }
> >   
> >       public static void main(String[] args) {
> > -        List<InetAddress> addrs = getAddressesForLocalHost();
> > -        if (addrs.size() < 2) {
> > +        List<InetAddress> addrs = getNonLoopbackAddressesForLocalHost();
> > +        if (addrs.size() < 1) {
> >               System.out.println("Ignoring manual test since no more than one IPs are configured for 'localhost'");
> >               return;
> >           }
> > @@ -186,7 +186,7 @@
> >           System.out.println("All tests PASSED.");
> >       }
> >   
> > -    private static List<InetAddress> getAddressesForLocalHost() {
> > +    private static List<InetAddress> getNonLoopbackAddressesForLocalHost() {
> >   
> >           try {
> >               return NetworkInterface.networkInterfaces()
> > 
> > 
> > Testing: Manual testing on local setup. jdk/submit (currently running)
> > 
> > Thanks,
> > Severin
> > 
> > 
> > [1] $ grep localhost /etc/hosts | grep -v '::'
> > 127.0.0.1    localhost localhost.localdomain localhost4 localhost4.localdomain4
> > 192.168.1.18 localhost
> >