Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8198317: Enhance JavacTool.getTask for flexibility #1896

Closed
wants to merge 4 commits into from

Conversation

@lgxbslgx
Copy link
Member

@lgxbslgx lgxbslgx commented Dec 25, 2020

Hi all,

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

Thank you for taking the time to review.

Best Regards.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8198317: Enhance JavacTool.getTask for flexibility

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1896/head:pull/1896
$ git checkout pull/1896

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 25, 2020

👋 Welcome back lgxbslgx! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk openjdk bot added the rfr label Dec 25, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 25, 2020

@lgxbslgx The following label will be automatically applied to this pull request:

  • compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Loading

@openjdk openjdk bot added the compiler label Dec 25, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 25, 2020

Loading

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

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

Loading

@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Dec 26, 2020

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.

Loading

@jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Dec 26, 2020

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?

Loading

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

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.

Loading

ToolProvider.getSystemJavaCompiler()
.getTask(null, null, null, Arrays.asList("-XDrawDiagnostics", "-Xlint:serial"), null, files)
.call();
tb.checkEqual(expected, Arrays.asList(bais.toString().split(lineSeparator)));
System.setErr(prev);
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Dec 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member Author

@lgxbslgx lgxbslgx Dec 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Loading

List<? extends JavaFileObject> files = Arrays.asList(new MemFile("Test.java", code));

// Situation: out is null and the value is not set in the context.
ByteArrayOutputStream bais = new ByteArrayOutputStream();
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Dec 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Loading

Copy link
Member Author

@lgxbslgx lgxbslgx Dec 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

@jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Dec 29, 2020

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

Loading

@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Jan 5, 2021

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

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 2, 2021

@lgxbslgx This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 2, 2021

@lgxbslgx This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it!

Loading

@bridgekeeper bridgekeeper bot closed this Mar 2, 2021
@lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Mar 2, 2021

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

Loading

@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Apr 6, 2021

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

Loading

@lgxbslgx lgxbslgx deleted the JDK-8198317 branch Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants