RFR: 8198317: Enhance JavacTool.getTask for flexibility

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8198317: Enhance JavacTool.getTask for flexibility

Guoxiong Li-2
Hi all,

This little patch enhances the setting of `Log` in `JavacTool.getTask`.

Thank you for taking the time to review.

Best Regards.

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

Commit messages:
 - 8198317: Enhance JavacTool.getTask for flexibility

Changes: https://git.openjdk.java.net/jdk/pull/1896/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1896&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8198317
  Stats: 8 lines in 1 file changed: 6 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1896.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1896/head:pull/1896

PR: https://git.openjdk.java.net/jdk/pull/1896
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8198317: Enhance JavacTool.getTask for flexibility

Jonathan Gibbons-2
On Fri, 25 Dec 2020 16:26:39 GMT, Guoxiong Li <[hidden email]> wrote:

> Hi all,
>
> This little patch enhances the setting of `Log` in `JavacTool.getTask`.
>
> Thank you for taking the time to review.
>
> Best Regards.

As a general rule, all changes should have an accompanying regression test, unless there is a good reason why not.   The set of good reasons has a corresponding list of JBS labels, shown here: http://openjdk.java.net/guide/#noreg

The closest match here would be `noreg-trivial` but I don't think it is close enough to claim that. In other words, I think this change needs a corresponding new test.  It is also not uncommon for small changes like this to more effort to write the test than fixing the underlying issue.

--

* *Unit test*: insurance against having to writing a regression test later on
* *Regression test*: the penalty for not writing a unit test in the first place

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

Changes requested by jjg (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1896
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8198317: Enhance JavacTool.getTask for flexibility [v2]

Guoxiong Li-2
In reply to this post by Guoxiong Li-2
> Hi all,
>
> This little patch enhances the setting of `Log` in `JavacTool.getTask`.
>
> Thank you for taking the time to review.
>
> Best Regards.

Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:

  Add a test case.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1896/files
  - new: https://git.openjdk.java.net/jdk/pull/1896/files/5ac98f79..f73c4052

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1896&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1896&range=00-01

  Stats: 74 lines in 1 file changed: 74 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1896.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1896/head:pull/1896

PR: https://git.openjdk.java.net/jdk/pull/1896
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8198317: Enhance JavacTool.getTask for flexibility [v2]

Guoxiong Li-2
In reply to this post by Jonathan Gibbons-2
On Fri, 25 Dec 2020 20:51:43 GMT, Jonathan Gibbons <[hidden email]> wrote:

>> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Add a test case.
>
> As a general rule, all changes should have an accompanying regression test, unless there is a good reason why not.   The set of good reasons has a corresponding list of JBS labels, shown here: http://openjdk.java.net/guide/#noreg
>
> The closest match here would be `noreg-trivial` but I don't think it is close enough to claim that. In other words, I think this change needs a corresponding new test.  It is also not uncommon for small changes like this to more effort to write the test than fixing the underlying issue.
>
> --
>
> * *Unit test*: insurance against having to writing a regression test later on
> * *Regression test*: the penalty for not writing a unit test in the first place

I submit a test case. But I don't satisfy with it, because it can't meet all the situations.
I try to use the reflect api locally to write a better test. Please see the code below. The code can't be compiled.

    @Test
    public void testLogSettingInJavacTool() throws Exception {
        Class<?> pwClass = Class.forName("java.io.PrintWriter");
        Field psOutField = pwClass.getDeclaredField("psOut");
        psOutField.setAccessible(true);
        Field outField = pwClass.getDeclaredField("out");
        outField.setAccessible(true);

        // Situation: out is null and the value is not set in the context.
        JavacTaskImpl task1 = (JavacTaskImpl) ToolProvider
                .getSystemJavaCompiler()
                .getTask(null, null, null, null, null, null);
        PrintWriter writer1 = task1.getContext().get(Log.errKey);
        PrintStream psOut1 = (PrintStream) psOutField.get(writer1);
        if (!System.err.equals(psOut1)) {
            throw new Error("The PrintWriter is set uncorrectly.");
        }

        // Situation: out is not null and out is a PrintWriter.
        PrintWriter expectedPW = new PrintWriter(System.out);
        JavacTaskImpl task2 = (JavacTaskImpl) ToolProvider
                .getSystemJavaCompiler()
                .getTask(expectedPW, null, null, null, null, null);
        PrintWriter writer2 = task2.getContext().get(Log.errKey);
        PrintStream psOut2 = (PrintStream) psOutField.get(writer2);
        if (!System.out.equals(psOut2) || expectedPW.equals(writer2)) {
            throw new Error("The PrintWriter is set uncorrectly.");
        }

        // Situation: out is not null and out is not a PrintWriter.
        OutputStreamWriter expectedOSW = new OutputStreamWriter(System.out);
        JavacTaskImpl task3 = (JavacTaskImpl) ToolProvider
                .getSystemJavaCompiler()
                .getTask(expectedOSW, null, null, null, null, null);
        PrintWriter writer3 = task3.getContext().get(Log.errKey);
        Writer out3 = (Writer) outField.get(writer3);
        if (!(out3 instanceof OutputStreamWriter) || !expectedOSW.equals(out3)) {
            throw new Error("The PrintWriter is set uncorrectly.");
        }
    }

The error information is shown below.

STDERR:
test: testLogSettingInJavacTool
Exception running test testLogSettingInJavacTool: java.lang.reflect.InaccessibleObjectException: Unable to make field private java.io.PrintStream java.io.PrintWriter.psOut accessible: module java.base does not "opens java.io" to unnamed module @2fd66ad3
java.lang.reflect.InaccessibleObjectException: Unable to make field private java.io.PrintStream java.io.PrintWriter.psOut accessible: module java.base does not "opens java.io" to unnamed module @2fd66ad3
        at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:357)
        at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
        at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:177)
        at java.base/java.lang.reflect.Field.setAccessible(Field.java:171)
        at T8198317.testLogSettingInJavacTool(T8198317.java:63)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:567)
        at toolbox.TestRunner.runTests(TestRunner.java:89)
        at toolbox.TestRunner.runTests(TestRunner.java:73)
        at T8198317.main(T8198317.java:56)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:567)
        at com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:298)
        at java.base/java.lang.Thread.run(Thread.java:831)

A better test case would be appreciated.

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

PR: https://git.openjdk.java.net/jdk/pull/1896
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8198317: Enhance JavacTool.getTask for flexibility [v2]

Jonathan Gibbons-2
On Sat, 26 Dec 2020 17:30:58 GMT, Guoxiong Li <[hidden email]> wrote:

>> As a general rule, all changes should have an accompanying regression test, unless there is a good reason why not.   The set of good reasons has a corresponding list of JBS labels, shown here: http://openjdk.java.net/guide/#noreg
>>
>> The closest match here would be `noreg-trivial` but I don't think it is close enough to claim that. In other words, I think this change needs a corresponding new test.  It is also not uncommon for small changes like this to more effort to write the test than fixing the underlying issue.
>>
>> --
>>
>> * *Unit test*: insurance against having to writing a regression test later on
>> * *Regression test*: the penalty for not writing a unit test in the first place
>
> I submit a test case. But I don't satisfy with it, because it can't meet all the situations.
> I try to use the reflect api locally to write a better test. Please see the code below. The code can't be compiled.
>
>     @Test
>     public void testLogSettingInJavacTool() throws Exception {
>         Class<?> pwClass = Class.forName("java.io.PrintWriter");
>         Field psOutField = pwClass.getDeclaredField("psOut");
>         psOutField.setAccessible(true);
>         Field outField = pwClass.getDeclaredField("out");
>         outField.setAccessible(true);
>
>         // Situation: out is null and the value is not set in the context.
>         JavacTaskImpl task1 = (JavacTaskImpl) ToolProvider
>                 .getSystemJavaCompiler()
>                 .getTask(null, null, null, null, null, null);
>         PrintWriter writer1 = task1.getContext().get(Log.errKey);
>         PrintStream psOut1 = (PrintStream) psOutField.get(writer1);
>         if (!System.err.equals(psOut1)) {
>             throw new Error("The PrintWriter is set uncorrectly.");
>         }
>
>         // Situation: out is not null and out is a PrintWriter.
>         PrintWriter expectedPW = new PrintWriter(System.out);
>         JavacTaskImpl task2 = (JavacTaskImpl) ToolProvider
>                 .getSystemJavaCompiler()
>                 .getTask(expectedPW, null, null, null, null, null);
>         PrintWriter writer2 = task2.getContext().get(Log.errKey);
>         PrintStream psOut2 = (PrintStream) psOutField.get(writer2);
>         if (!System.out.equals(psOut2) || expectedPW.equals(writer2)) {
>             throw new Error("The PrintWriter is set uncorrectly.");
>         }
>
>         // Situation: out is not null and out is not a PrintWriter.
>         OutputStreamWriter expectedOSW = new OutputStreamWriter(System.out);
>         JavacTaskImpl task3 = (JavacTaskImpl) ToolProvider
>                 .getSystemJavaCompiler()
>                 .getTask(expectedOSW, null, null, null, null, null);
>         PrintWriter writer3 = task3.getContext().get(Log.errKey);
>         Writer out3 = (Writer) outField.get(writer3);
>         if (!(out3 instanceof OutputStreamWriter) || !expectedOSW.equals(out3)) {
>             throw new Error("The PrintWriter is set uncorrectly.");
>         }
>     }
>
> The error information is shown below.
>
> STDERR:
> test: testLogSettingInJavacTool
> Exception running test testLogSettingInJavacTool: java.lang.reflect.InaccessibleObjectException: Unable to make field private java.io.PrintStream java.io.PrintWriter.psOut accessible: module java.base does not "opens java.io" to unnamed module @2fd66ad3
> java.lang.reflect.InaccessibleObjectException: Unable to make field private java.io.PrintStream java.io.PrintWriter.psOut accessible: module java.base does not "opens java.io" to unnamed module @2fd66ad3
> at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:357)
> at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
> at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:177)
> at java.base/java.lang.reflect.Field.setAccessible(Field.java:171)
> at T8198317.testLogSettingInJavacTool(T8198317.java:63)
> at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
> at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.base/java.lang.reflect.Method.invoke(Method.java:567)
> at toolbox.TestRunner.runTests(TestRunner.java:89)
> at toolbox.TestRunner.runTests(TestRunner.java:73)
> at T8198317.main(T8198317.java:56)
> at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
> at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.base/java.lang.reflect.Method.invoke(Method.java:567)
> at com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:298)
> at java.base/java.lang.Thread.run(Thread.java:831)
>
> A better test case would be appreciated.

While it is acceptable practice for file manager tests to access the file manager internals, and more generally for javac tests to access the javac internals, it is less good to access the internals of other parts of the system (like PrintWriter in java.base), since future changes to those components could break this test as an unwelcome side effect.

How easy would it be to observe/verify the desired behavior by monitoring the output to the resulting PrintWriter object, e.g. by triggering some known diagnostic?

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

PR: https://git.openjdk.java.net/jdk/pull/1896
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8198317: Enhance JavacTool.getTask for flexibility [v3]

Guoxiong Li-2
In reply to this post by Guoxiong Li-2
> Hi all,
>
> This little patch enhances the setting of `Log` in `JavacTool.getTask`.
>
> Thank you for taking the time to review.
>
> Best Regards.

Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:

  Revise test.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1896/files
  - new: https://git.openjdk.java.net/jdk/pull/1896/files/f73c4052..e13c072b

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1896&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1896&range=01-02

  Stats: 63 lines in 1 file changed: 52 ins; 0 del; 11 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1896.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1896/head:pull/1896

PR: https://git.openjdk.java.net/jdk/pull/1896
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8198317: Enhance JavacTool.getTask for flexibility [v3]

Jonathan Gibbons-2
On Sat, 26 Dec 2020 18:58:15 GMT, Guoxiong Li <[hidden email]> wrote:

>> Hi all,
>>
>> This little patch enhances the setting of `Log` in `JavacTool.getTask`.
>>
>> Thank you for taking the time to review.
>>
>> Best Regards.
>
> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
>
>   Revise test.

Any time that tests split a string into lines it is really important to ensure that it works on Windows as well as Linux/Mac platforms, because of the different line endings.

The code looks OK. Alternatively, if you are not concerned to check for the exact line ending sequence, you can use `split("\\R")` to split on any line ending sequence.  Either way, it's worth checking the test runs on platforms with both kinds of typical line ending.

test/langtools/tools/javac/api/8198317/T8198317.java line 90:

> 88:                 .call();
> 89:         tb.checkEqual(expected, Arrays.asList(bais.toString().split(lineSeparator)));
> 90:         System.setErr(prev);

Although not strictly necessary in this case, it is good practice to use `try ... finally` to reset values back to some previous value. In other words, the `System.setErr(prev);` should be in the `finally` clause.

test/langtools/tools/javac/api/8198317/T8198317.java line 82:

> 80:
> 81:         // Situation: out is null and the value is not set in the context.
> 82:         ByteArrayOutputStream bais = new ByteArrayOutputStream();

The variable name `bais` is "surprising". Is it a cut-n-paste from elsewhere?    I was expecting to see `baos` as an acronym for "byte array output stream",  `bais` suggests "byte array input stream".

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

PR: https://git.openjdk.java.net/jdk/pull/1896
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8198317: Enhance JavacTool.getTask for flexibility [v3]

Jonathan Gibbons-2
On Tue, 29 Dec 2020 03:40:52 GMT, Jonathan Gibbons <[hidden email]> wrote:

>> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Revise test.
>
> Any time that tests split a string into lines it is really important to ensure that it works on Windows as well as Linux/Mac platforms, because of the different line endings.
>
> The code looks OK. Alternatively, if you are not concerned to check for the exact line ending sequence, you can use `split("\\R")` to split on any line ending sequence.  Either way, it's worth checking the test runs on platforms with both kinds of typical line ending.

Given that the `langtools/tier1` pre-submit tests worked for Linux and Windows, you're OK on the "newline" front.

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

PR: https://git.openjdk.java.net/jdk/pull/1896
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8198317: Enhance JavacTool.getTask for flexibility [v4]

Guoxiong Li-2
In reply to this post by Guoxiong Li-2
> Hi all,
>
> This little patch enhances the setting of `Log` in `JavacTool.getTask`.
>
> Thank you for taking the time to review.
>
> Best Regards.

Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:

  Revise test code. Fix typo.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1896/files
  - new: https://git.openjdk.java.net/jdk/pull/1896/files/e13c072b..909140e1

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1896&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1896&range=02-03

  Stats: 11 lines in 1 file changed: 3 ins; 0 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1896.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1896/head:pull/1896

PR: https://git.openjdk.java.net/jdk/pull/1896
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8198317: Enhance JavacTool.getTask for flexibility [v3]

Guoxiong Li-2
In reply to this post by Jonathan Gibbons-2
On Tue, 29 Dec 2020 03:31:08 GMT, Jonathan Gibbons <[hidden email]> wrote:

>> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Revise test.
>
> test/langtools/tools/javac/api/8198317/T8198317.java line 90:
>
>> 88:                 .call();
>> 89:         tb.checkEqual(expected, Arrays.asList(bais.toString().split(lineSeparator)));
>> 90:         System.setErr(prev);
>
> Although not strictly necessary in this case, it is good practice to use `try ... finally` to reset values back to some previous value. In other words, the `System.setErr(prev);` should be in the `finally` clause.

Fixed.

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

PR: https://git.openjdk.java.net/jdk/pull/1896
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8198317: Enhance JavacTool.getTask for flexibility [v3]

Guoxiong Li-2
In reply to this post by Jonathan Gibbons-2
On Tue, 29 Dec 2020 03:34:21 GMT, Jonathan Gibbons <[hidden email]> wrote:

>> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Revise test.
>
> test/langtools/tools/javac/api/8198317/T8198317.java line 82:
>
>> 80:
>> 81:         // Situation: out is null and the value is not set in the context.
>> 82:         ByteArrayOutputStream bais = new ByteArrayOutputStream();
>
> The variable name `bais` is "surprising". Is it a cut-n-paste from elsewhere?    I was expecting to see `baos` as an acronym for "byte array output stream",  `bais` suggests "byte array input stream".

Fixed. I used `ByteArrayInputStream bais = new ByteArrayInputStream();` at the beginning. Then I identified the `ByteArrayInputStream` is not right and changed to use `ByteArrayOutputStream`. But I forgot to revise the variable name `bais`. Sorry for wasting the time at this careless detail.

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

PR: https://git.openjdk.java.net/jdk/pull/1896
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8198317: Enhance JavacTool.getTask for flexibility [v3]

Guoxiong Li-2
In reply to this post by Jonathan Gibbons-2
On Tue, 29 Dec 2020 03:43:14 GMT, Jonathan Gibbons <[hidden email]> wrote:

>> Any time that tests split a string into lines it is really important to ensure that it works on Windows as well as Linux/Mac platforms, because of the different line endings.
>>
>> The code looks OK. Alternatively, if you are not concerned to check for the exact line ending sequence, you can use `split("\\R")` to split on any line ending sequence.  Either way, it's worth checking the test runs on platforms with both kinds of typical line ending.
>
> Given that the `langtools/tier1` pre-submit tests worked for Linux and Windows, you're OK on the "newline" front.

@jonathan-gibbons I updated the code according to your comments. Thank you for taking the time to review.

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

PR: https://git.openjdk.java.net/jdk/pull/1896
Reply | Threaded
Open this post in threaded view
|

Withdrawn: 8198317: Enhance JavacTool.getTask for flexibility

duke
In reply to this post by Guoxiong Li-2
On Fri, 25 Dec 2020 16:26:39 GMT, Guoxiong Li <[hidden email]> wrote:

> Hi all,
>
> This little patch enhances the setting of `Log` in `JavacTool.getTask`.
>
> Thank you for taking the time to review.
>
> Best Regards.

This pull request has been closed without being integrated.

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

PR: https://git.openjdk.java.net/jdk/pull/1896
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8198317: Enhance JavacTool.getTask for flexibility [v3]

Jan Lahoda-3
In reply to this post by Guoxiong Li-2
On Tue, 5 Jan 2021 14:29:25 GMT, Guoxiong Li <[hidden email]> wrote:

>> Given that the `langtools/tier1` pre-submit tests worked for Linux and Windows, you're OK on the "newline" front.
>
> @jonathan-gibbons I updated the code according to your comments. Thank you for taking the time to review.

@lgxbslgx, could you please re-open this PR? Thanks!

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

PR: https://git.openjdk.java.net/jdk/pull/1896
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8198317: Enhance JavacTool.getTask for flexibility [v3]

Guoxiong Li-2
On Tue, 2 Mar 2021 17:43:28 GMT, Jan Lahoda <[hidden email]> wrote:

>> @jonathan-gibbons I updated the code according to your comments. Thank you for taking the time to review.
>
> @lgxbslgx, could you please re-open this PR? Thanks!

@lahodaj sorry for the delay. I will assist you to completing the new PR.

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

PR: https://git.openjdk.java.net/jdk/pull/1896