-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8361613: System.console() should only be available for interactive terminal #26273
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,33 +21,66 @@ | |
* questions. | ||
*/ | ||
|
||
import org.junit.jupiter.api.Test; | ||
import static org.junit.jupiter.api.Assertions.*; | ||
import java.nio.file.Files; | ||
import java.nio.file.Paths; | ||
|
||
import jdk.test.lib.process.OutputAnalyzer; | ||
import jdk.test.lib.process.ProcessTools; | ||
import org.junit.jupiter.api.Assumptions; | ||
import org.junit.jupiter.api.BeforeAll; | ||
import org.junit.jupiter.params.ParameterizedTest; | ||
import org.junit.jupiter.params.provider.ValueSource; | ||
|
||
import static jdk.test.lib.Utils.*; | ||
|
||
/** | ||
* @test | ||
* @bug 8341975 8351435 | ||
* @bug 8341975 8351435 8361613 | ||
* @summary Tests the default charset. It should honor `stdout.encoding` | ||
* which should be the same as System.out.charset() | ||
* @modules jdk.internal.le | ||
* @run junit/othervm -Djdk.console=jdk.internal.le -Dstdout.encoding=UTF-8 DefaultCharsetTest | ||
* @run junit/othervm -Djdk.console=jdk.internal.le -Dstdout.encoding=ISO-8859-1 DefaultCharsetTest | ||
* @run junit/othervm -Djdk.console=jdk.internal.le -Dstdout.encoding=US-ASCII DefaultCharsetTest | ||
* @run junit/othervm -Djdk.console=jdk.internal.le -Dstdout.encoding=foo DefaultCharsetTest | ||
* @run junit/othervm -Djdk.console=jdk.internal.le DefaultCharsetTest | ||
* @requires (os.family == "linux") | (os.family == "mac") | ||
* @library /test/lib | ||
* @build jdk.test.lib.Utils | ||
* jdk.test.lib.JDKToolFinder | ||
* jdk.test.lib.process.ProcessTools | ||
* @run junit DefaultCharsetTest | ||
*/ | ||
public class DefaultCharsetTest { | ||
@Test | ||
public void testDefaultCharset() { | ||
@BeforeAll | ||
static void checkExpectAvailability() { | ||
// check "expect" command availability | ||
var expect = Paths.get("/usr/bin/expect"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We only need to check "expect" availability once, so we should move this check to a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Modified to use |
||
Assumptions.assumeTrue(Files.exists(expect) && Files.isExecutable(expect), | ||
"'" + expect + "' not found. Test ignored."); | ||
} | ||
@ParameterizedTest | ||
@ValueSource(strings = {"UTF-8", "ISO-8859-1", "US-ASCII", "foo", ""}) | ||
void testDefaultCharset(String stdoutEncoding) throws Exception { | ||
// invoking "expect" command | ||
OutputAnalyzer oa = ProcessTools.executeProcess( | ||
"expect", | ||
"-n", | ||
TEST_SRC + "/defaultCharset.exp", | ||
TEST_CLASSES, | ||
TEST_JDK + "/bin/java", | ||
"-Dstdout.encoding=" + stdoutEncoding, | ||
getClass().getName()); | ||
oa.reportDiagnosticSummary(); | ||
oa.shouldHaveExitValue(0); | ||
} | ||
|
||
public static void main(String... args) { | ||
var stdoutEncoding = System.getProperty("stdout.encoding"); | ||
var sysoutCharset = System.out.charset(); | ||
var consoleCharset = System.console().charset(); | ||
System.out.println(""" | ||
stdout.encoding = %s | ||
System.out.charset() = %s | ||
System.console().charset() = %s | ||
""".formatted(stdoutEncoding, sysoutCharset.name(), consoleCharset.name())); | ||
assertEquals(consoleCharset, sysoutCharset, | ||
"Charsets for System.out and Console differ for stdout.encoding: %s".formatted(stdoutEncoding)); | ||
System.out.printf(""" | ||
stdout.encoding = %s | ||
System.out.charset() = %s | ||
System.console().charset() = %s | ||
""", stdoutEncoding, sysoutCharset.name(), consoleCharset.name()); | ||
if (!consoleCharset.equals(sysoutCharset)) { | ||
System.err.printf("Charsets for System.out and Console differ for stdout.encoding: %s%n", stdoutEncoding); | ||
System.exit(-1); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method declaration already links to Console so I don't think we need another link in the "See also" section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I missed it, but do we have anything to make it clear that it returns null if either stdin or stdout are redirected?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do have wordings like " If the virtual machine is started from an interactive command line without redirecting the standard input AND output streams then its console will exist ..." and "If no console device is
available then an invocation of that method will return null" from the very beginning. not very "straightforward" but i think it's clear enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding
@see
tag would be more helpful.As to the spec wording wrt stdin/out, there is another issue filed to make it clearer: JDK-8361972. This PR addresses the implementation part only so that it can be backported to prior LTSes without spec change.