RFR: 8291226: Create Test Cases to cover scenarios for JDK-8278067 [v3]

[Daniel Fuchs]

On Mon, 22 Aug 2022 17:18:21 GMT, Ramesh Bhagavatam Gangadhar <rgangadhar at openjdk.org> wrote:

>> There are total 160 scenarios written with combination of client properties (Client Scenarios) and Server Response (Server Scenarios).
>> 
>> In tabular format, Client and Server scenarios along with expected output are documented here:[Permalink](https://bugs.openjdk.org/browse/JDK-8291226?focusedCommentId=14519074&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14519074)
>> 
>> This Program Should be run mandatorily in othervm mode itself since it has system property changes so can't be clubbed with other scenarios. so each scenario should be run in individual JVM.
>> 
>> For each and every scenario, ServerSocket is created and waits for clients to connect to it.
>> isProxySet and serverReady are shared variables between server thread and client thread(main) and it should be set and reset to false for each and every scenario.
>> 
>> isProxySet and serverReady variables should be set by server thread before proceeding to client thread(main).
>> 
>> if isProxySet variable is set to true then client set the proxy value to url.openConnection(Proxy)
>> <SNIPPET>
>> if (isProxySet) { 
>>          httpUrlConnection = (sun.net.www.protocol.http.HttpURLConnection) url .openConnection(new Proxy(Type.HTTP, new InetSocketAddress("localhost", SERVER_PORT))); } 
>> else { 
>>         httpUrlConnection = (sun.net.www.protocol.http.HttpURLConnection) url.openConnection(); 
>> }
>> </SNIPPET>
>> 
>> Program tries to fetch the Value of <Key, Value> Pairs of HashMap KeepAliveCache where Key is KeepAliveKey and Value is ClientVector KeepAliveTimeout is stored in Value ClientVector of HashMap KeepAliveCache.
>> 
>> if connection is cached then KeepAliveTimeout is stored in ClientVector. KeepAliveTimeout stored in Value(ClientVector) of HashMap KeepAliveCache is compared with Expected Value.
>> 
>> if connection is not cached then connection is terminated immediately.
>
> Ramesh Bhagavatam Gangadhar has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update and rename KeepAliveTest to KeepAliveTest.java
>   
>   1. Increased space from 2 to 4.
>   2. skipped test scenarios from 113 to 128 && 145 to 160

Changes requested by dfuchs (Reviewer).

test/jdk/sun/net/www/http/HttpClient/KeepAliveTest.java line 263:

> 261:     private static final String H = C + NEW_LINE + KEEP_ALIVE_TIMEOUT_NEG;
> 262:     private static final String I = C + NEW_LINE + KEEP_ALIVE_TIMEOUT_ZERO;
> 263:     private static Logger logger = Logger.getLogger("sun.net.www.protocol.http.HttpURLConnection");

should be final

test/jdk/sun/net/www/http/HttpClient/KeepAliveTest.java line 264:

> 262:     private static final String I = C + NEW_LINE + KEEP_ALIVE_TIMEOUT_ZERO;
> 263:     private static Logger logger = Logger.getLogger("sun.net.www.protocol.http.HttpURLConnection");
> 264:     private String[] serverScenarios = {

should be final

test/jdk/sun/net/www/http/HttpClient/KeepAliveTest.java line 279:

> 277:      * following are client scenarios which are repeated.
> 278:      */
> 279:     private String[] a = {

should be final

test/jdk/sun/net/www/http/HttpClient/KeepAliveTest.java line 287:

> 285:         SERVER_100 + CLIENT_SEPARATOR + PROXY_200_NEGATIVE, SERVER_100_NEGATIVE + CLIENT_SEPARATOR + PROXY_200
> 286:     };
> 287:     private String[] clientScenarios = {

should be final

test/jdk/sun/net/www/http/HttpClient/KeepAliveTest.java line 299:

> 297:         a[0] , a[1], a[2], a[3], a[4], a[5], a[6], a[7], a[8], a[9], a[10], a[11], a[12], a[13], a[14], a[15],
> 298:     };
> 299:     private int[] expectedValues = {

should be final

test/jdk/sun/net/www/http/HttpClient/KeepAliveTest.java line 311:

> 309:         0,    0,    0,   0,  0,  0,  0,  0,  0,   0,   0,   0,    0,    0,     0,    0,
> 310:     };
> 311:     private CountDownLatch countDownLatch = null;

should be final and initialized to `new CountDownLatch(1);`

test/jdk/sun/net/www/http/HttpClient/KeepAliveTest.java line 317:

> 315:         if((scenarioNumber >= 112 && scenarioNumber <= 127) || (scenarioNumber >= 144 && scenarioNumber <= 159)) {
> 316:             System.out.println("Scenario Skipped:"+scenarioNumber);
> 317:             countDownLatch = new CountDownLatch(0);

should count down the latch instead of replacing its value

test/jdk/sun/net/www/http/HttpClient/KeepAliveTest.java line 328:

> 326:         }
> 327:         System.out.println();
> 328:         serverInit(scenarioNumber);

should probably be called `startServer`

test/jdk/sun/net/www/http/HttpClient/KeepAliveTest.java line 329:

> 327:         System.out.println();
> 328:         serverInit(scenarioNumber);
> 329:         clientInit(scenarioNumber);

should probably be called `runClient`

test/jdk/sun/net/www/http/HttpClient/KeepAliveTest.java line 466:

> 464: 
> 465:         // wait until ServerSocket moves to listening state.
> 466:         while (!serverReady) {

Could use a cyclic barrier or a count down latch instead of sleeping here.

test/jdk/sun/net/www/http/HttpClient/KeepAliveTest.java line 571:

> 569:                 }
> 570:             }
> 571:         }

There is an alignment problem here: it looks as if a close brace is missing,

test/jdk/sun/net/www/http/HttpClient/KeepAliveTest.java line 576:

> 574:             System.out.println("Usage:java KeepAliveTest.java <scenarioNumber>");
> 575:             System.out.println("e.g.:java KeepAliveTest.java 1");
> 576:             System.exit(1);

Since this is a test I would advise throwing an `IllegalArgumentException` or `AssertionError` instead of calling `System.exit(1);` Though arguably it doesn't matter much since this test must run in `/othervm` mode.

test/jdk/sun/net/www/http/HttpClient/KeepAliveTest.java line 584:

> 582:         KeepAliveTest keepAliveTest = new KeepAliveTest();
> 583:         if (args.length != 0) {
> 584:             keepAliveTest.myinit(Integer.valueOf(args[0]));

It would be good to have method names that reflect what the method does. Here for instance - what `myinit` does is actually running (or starting) the given scenario. So maybe it should be called `startScenario`.

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

PR: https://git.openjdk.org/jdk/pull/9958