On Wed, 10 Apr 2024 10:40:55 GMT, Jaikiran Pai <j...@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
