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

[Ramesh Bhagavatam Gangadhar]

On Mon, 22 Aug 2022 17:54:30 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> 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
>
> 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

done thanks

> 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

now serverScenarios is retrieved using getServerScenarion(int) and serverScenarios array is commented.

> 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

done thanks

> 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

replaced with this.countDownLatch.countDown(). thanks

> test/jdk/sun/net/www/http/HttpClient/KeepAliveTest.java line 328:
> 
>> 326:         }
>> 327:         System.out.println();
>> 328:         serverInit(scenarioNumber);
> 
> should probably be called `startServer`

changed method name to startServer. thanks

> 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`

changed method name to runClient. thanks

> 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.

Used serverCountDownLatch @dfuch thanks.

> 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,

aligned properly. thanks

> 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.

replaced System.exit(1) with throw new IllegalArgumentException("Usage:java KeepAliveTest.java <scenarioNumber>"); thanks

> 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`.

modified name to startScenario thanks @dfuch

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

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