[RFR]: 8220575: Replace hardcoded 127.0.0.1 in URLs with new URI builder

[Daniel Fuchs]

Hi Arthur,

I believe this looks good. Here are my comments so far:

I like the fact that you kept the builder implementation
very minimal, and focused on what these tests actually
need. We can always revisit that later if we come across
new tests that need more than what your proposed implementation
currently offer.

There are a couple of things that would need to be fixed in your
webrev though:

1. There are some places where the root path "/" is replaced by
    no path - so the proposed changes may change the test in
    subtle ways - which I'd rather take out of the picture.

    For instance - the following files seem to
    be missing a `.path("/")` line:

    ExpiredCookieTest.java, NoProxyTest.java, NullSelector.java

    For these three files the orginal URL ended with "/", but
    as per your changes the new URL won't.

2. B6890349.java

    Using URI.toURL() in this test will change the nature of
    the test. I believe that in this specific case using the
    multi-arg URL constuctor should be preferred.
    The reason is that URI as per its specification encodes
    illegal characters, while URL as per its specification,
    does not. So going through URI here will lead to a
    different URL than the one that the test is testing.

    I see from your mail that you have noticed the difference :-)

    For this one test I'd suggest to make an exception and
    change the URL construction to:

    URL u = new URL("http",
                    InetAddress.getLoopbackAddress().getHostAddress(),
                    port,
                    "/foo\nbar");

    Even if we get to deprecating the URL constructors in the
    future this would still be a valid case for using them in
    a test.

3. on the 'nit' side it's a bit strange to see
    the sequence of calls `.host(..).path(..).port(..)`,
    as `.host(..).port(..).path(..)` would feel
    natural - but don't feel obliged to make this change.

best regards,

-- daniel

On 25/03/2019 22:18, Arthur Eubanks wrote:
> Hi,
> 
> This patch replaces hardcoded "http://127.0.0.1/" in tests with a new 
> URIBuilder and InetAddress.getLoopbackAddress().getHostAddress(). 
> Creating a URIBuilder helper class was discussed in a previous thread on 
> net-dev.
> 
> http://cr.openjdk.java.net/~aeubanks/8220575/webrev.00/index.html
> 
> One problem I came across was in 
> test/jdk/sun/net/www/protocol/http/B6890349.java, where "/foo\nbar" in a 
> URL is not the same as "/foo\nbar" in a URI, so I'd like some help in 
> that one test (you'll see some print statements in there, which produce 
> different output). Otherwise, the other tests pass locally.