On Tue, 25 Feb 2025 08:57:00 GMT, SendaoYan <s...@openjdk.org> wrote:

> Hi all,
> 
> Two java/net/InetAddress tests fails "java.net.UnknownHostException" on some 
> special machines. The machine cannot connect to the Internet directly and 
> needs to be connected to the Internet through a jump machine. The 
> java/net/InetAddress tests report "java.net.UnknownHostException: 
> bugs.openjdk.org: Temporary failure in name resolution" failure do not caused 
> by JVM bug but environmentalk issue, so this PR match the 
> java.net.UnknownHostException and throw jtreg.SkippedException instead of 
> report test fails.
> 
> And run the tests with JVM options `-Dhttp.proxyHost=192.168.50.1 
> -Dhttp.proxyPort=10991 -Dhttps.proxyHost=192.168.50.1 -Dhttps.proxyPort=10991 
> -DsocksProxyHost=192.168.50.1 -DsocksProxyPort=10991` can not work around. So 
> I create this PR to fix the test bug.
> 
> Command wget shows that the machine should connect to internet through a 
> jumper machine(192.168.50.1:10991):
> 
>> wget www.bing.com
> --2025-02-24 17:56:25--  http://www.bing.com/
> Connecting to 192.168.50.1:10991... connected.
> Proxy request sent, awaiting response... 301 Moved Permanently
> Location: http://cn.bing.com/ [following]
> --2025-02-24 17:56:25--  http://cn.bing.com/
> Reusing existing connection to 192.168.50.1:10991.
> Proxy request sent, awaiting response... 200 OK
> Length: 13006 (13K) [text/html]
> Saving to: ‘index.html’
> 
> 
> Change has been verified locally, test-fix only and make tests more 
> robustness, no risk.

Overall looks good to me, just minor cleanup comments and a question about 
`@build jtreg.SkippedException`

test/jdk/java/net/InetAddress/IsReachableViaLoopbackTest.java line 26:

> 24: import java.io.*;
> 25: import java.net.*;
> 26: import java.util.*;

I know, this is not a part of your change, but this import is not used. Could 
you please remove it?

test/jdk/java/net/InetAddress/IsReachableViaLoopbackTest.java line 35:

> 33:  * @key intermittent
> 34:  * @library /test/lib
> 35:  * @build jtreg.SkippedException

AFAIK,`@build jtreg.SkippedException` is not needed. Tried to remove and it 
worked fine locally, does it work for you?

Also this is the same for the line 30 in the `getOriginalHostName.java`

test/jdk/java/net/InetAddress/IsReachableViaLoopbackTest.java line 64:

> 62:         } catch (java.net.UnknownHostException e) {
> 63:             e.printStackTrace();
> 64:             throw new SkippedException("Network setup issue, skip this 
> test");

Minor: Do you think it would make the comment cleaner if it would be just 
`"Network setup issue"`? It seems to be repeating itself for me (skip exception 
already states, that the test is skipped). 

Don't have a strong opinion, so it's completely up you :)

Also this is the same for the line 60 in the `getOriginalHostName.java`

-------------

PR Review: https://git.openjdk.org/jdk/pull/23767#pullrequestreview-2653693064
PR Review Comment: https://git.openjdk.org/jdk/pull/23767#discussion_r1977263320
PR Review Comment: https://git.openjdk.org/jdk/pull/23767#discussion_r1977254299
PR Review Comment: https://git.openjdk.org/jdk/pull/23767#discussion_r1977283080

Reply via email to