On Fri, 8 Jul 2022 12:10:29 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> I removed a section of via JDK_JAVA_OPTIONS because including main class is
>> not allowed in the specification.
>> This behavior is added in JDK-8170832, which add JAVA_OPTIONS environment
>> variable. At this time, this test is mismatch with the specification.
>> I tried to test and get Passed on Japanese Windows environment.
>> Could you review this fix, please?
>
> The change looks fine to me since in its current form the usage of
> `JDK_JAVA_OPTIONS` is incorrect.
>
> Having said that and looking at the history of this file, I think, this part
> of the test was added to check if the `java` launcher would be able to read
> an environment variable (specifically the `JDK_JAVA_OPTIONS`) whose value was
> in `MS932` encoding, and pass it along as a correctly encoded String, all the
> way to the main method of the application. If we remove this section of the
> test, then we would be skipping that code path completely.
>
> Perhaps this part of the test could be modified a bit to let it pass the
> `JDK_JAVA_OPTIONS` environment variable with a `MS932` encoded value that
> acts some option to the java launcher, instead of being the argument to the
> main method? That way, you won't have to specify the main class name in the
> value of the environment variable. Specifically, something like:
>
>
> diff --git a/test/jdk/tools/launcher/I18NArgTest.java
> b/test/jdk/tools/launcher/I18NArgTest.java
> index fa09736da2f..2ba724d6f1d 100644
> --- a/test/jdk/tools/launcher/I18NArgTest.java
> +++ b/test/jdk/tools/launcher/I18NArgTest.java
> @@ -97,12 +97,17 @@ public class I18NArgTest extends TestHelper {
>
> // Test via JDK_JAVA_OPTIONS
> Map<String, String> env = new HashMap<>();
> - String cmd = "-Dtest.src=" + TEST_SOURCES_DIR.getAbsolutePath() +
> - " -Dtest.classes=" + TEST_CLASSES_DIR.getAbsolutePath() +
> - " -cp " + TEST_CLASSES_DIR.getAbsolutePath() +
> - " I18NArgTest " + unicodeStr + " " + hexValue;
> - env.put("JDK_JAVA_OPTIONS", cmd);
> - tr = doExec(env, javaCmd);
> + String sysPropName = "foo.bar";
> + // pass "-Dfoo.bar=<unicodestr>" through the JDK_JAVA_OPTIONS
> environment variable.
> + // we expect that system property value to be passed along to the
> main method with the
> + // correct encoding
> + String jdkJavaOpts = "-D" + sysPropName + "=" + unicodeStr;
> + env.put("JDK_JAVA_OPTIONS", jdkJavaOpts);
> + tr = doExec(env,javaCmd,
> + "-Dtest.src=" + TEST_SOURCES_DIR.getAbsolutePath(),
> + "-Dtest.classes=" + TEST_CLASSES_DIR.getAbsolutePath(),
> + "-cp", TEST_CLASSES_DIR.getAbsolutePath(),
> + "I18NArgTest", unicodeStr, hexValue, sysPropName);
> System.out.println(tr.testOutput);
> if (!tr.isOK()) {
> System.err.println(tr);
> @@ -125,5 +130,23 @@ public class I18NArgTest extends TestHelper {
> "expected:" + expected + " obtained:" + hexValue;
> throw new RuntimeException(message);
> }
> + if (args.length == 3) {
> + // verify the value of the system property matches the expected
> value
> + String sysPropName = args[2];
> + String sysPropVal = System.getProperty(sysPropName);
> + if (sysPropVal == null) {
> + throw new RuntimeException("Missing system property " +
> sysPropName);
> + }
> + String sysPropHexVal = "";
> + for (int i = 0; i < sysPropVal.length(); i++) {
> + sysPropHexVal =
> sysPropHexVal.concat(Integer.toHexString(sysPropVal.charAt(i)));
> + }
> + System.out.println("System property " + sysPropName + " computed
> hex value: "
> + + sysPropHexVal);
> + if (!sysPropHexVal.equals(expected)) {
> + throw new RuntimeException("Unexpected value in system
> property, expected "
> + + expected + ", but got " + sysPropHexVal);
> + }
> + }
> }
> }
>
> I haven't tested this change, so you might have to experiment with it a bit.
> What do you think?
@jaikiran
Thank you for your comment. I agree to the additional check.
I run the test you proposed, but it failed. I'm surveying that reason.
-------------
PR: https://git.openjdk.org/jdk/pull/9389
