RFR: 8319678: Several tests from corelibs areas ignore VM flags [v2]

[Mahendra Chhipa]

On Wed, 10 Apr 2024 10:40:55 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Mahendra Chhipa has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Implemented review comments.
>>   Updated EscapePath test.
>
> test/jdk/javax/naming/spi/providers/InitialContextTest.java line 65:
> 
>> 63:  * @library /test/lib
>> 64:  * @build jdk.test.lib.process.ProcessTools
>> 65:  * @run main/othervm InitialContextTest
> 
> Hello Mahendra, I see that this test and one other test is being changed to `/othervm` and I suspect it's because, we now call:
> 
> 
> System.setProperty("test.noclasspath", "true");
> 
> for the `ProcessTools` to skip adding the default `-cp` option to the launched Java process. 
> In reality, we don't have to set that system property and thus you don't have to change this and the other test to `othervm`. See a previous discussion about the classpath handling by `ProcessTools` here https://github.com/openjdk/jdk/pull/17787/files/a9fcc6c2900356250b29b3d11b402790a84d9317#r1484276695 - essentially, whatever classpath you end up passing as part of the command to `ProcessTools.createTestJavaProcessBuilder()` will end up getting used.

Thanks. Implemented in this commit.

> test/jdk/javax/naming/spi/providers/InitialContextTest.java line 268:
> 
>> 266:             OutputAnalyzer outputAnalyzer = ProcessTools.executeCommand(commands);
>> 267:             if(outputAnalyzer.getExitValue() != 0) {
>> 268:                 throw new RuntimeException(outputAnalyzer.getOutput());
> 
> I think it would be better to just call:
> 
> outputAnalyzer.shouldHaveExitValue(0);
> 
> that way it even prints the stdout/stderr logs and throws the exception.

Thanks. Implemented in this commit.

> test/jdk/javax/naming/spi/providers/InitialContextTest.java line 271:
> 
>> 269:             }
>> 270:         } catch (Exception ex) {
>> 271:             throw new RuntimeException(ex.getMessage());
> 
> This and other similar places in this PR will end up swallowing the underlying exception. It would be better to just have the method have a `throws Exception` or change this to:
> 
> 
> throw new RuntimeException(ex);

Thanks. Implemented in this commit.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18602#discussion_r1560681857
PR Review Comment: https://git.openjdk.org/jdk/pull/18602#discussion_r1560682373
PR Review Comment: https://git.openjdk.org/jdk/pull/18602#discussion_r1560683141