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

[Jonathan Gibbons]

On Sat, 26 Dec 2020 17:30:58 GMT, Guoxiong Li <github.com+13688759+lgxbslgx at openjdk.org> 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