[MSHARED-938] - #60
Conversation
add charset config
elharo
left a comment
elharo
left a comment
There was a problem hiding this comment.
Is it possible to test this fix?
|
Would it be helpful to add a test case in org.apache.maven.shared.utils.cli.CommandLineUtilsTest? |
|
|
elharo
left a comment
elharo
left a comment
There was a problem hiding this comment.
Yes, a unit test is helpful, especially when fixing a bug. It should fail without the PR and pass with this PR. That was we know the bug is actually fixed.
Add test unit to prove bug is fixed.
| public void testChineseEncodingIssue() | ||
| throws Exception | ||
| { | ||
| Commandline commandline = new Commandline( "ping www.baidu.com" ); |
There was a problem hiding this comment.
No, we cannot have tests which require outbound access. Espcially ICMP. Please change test.
Exclude ICMP in the test.
| } | ||
|
|
||
| outputPumper = new StreamPumper( p.getInputStream(), systemOut ); | ||
| outputPumper = new StreamPumper( p.getInputStream(), systemOut , streamCharset ); |
There was a problem hiding this comment.
Space before comma is not required.
| } | ||
|
|
||
| @Test | ||
| public void testChineseEncodingIssue() |
There was a problem hiding this comment.
This test is completely pointless:
- The
echowill use the encoding supplied by the entire system. There is no guarantee that GBK is the system encoding. - You never verify the output of th command to be what you expect.
What you need is an application that produces GBK bytes , those are read by Java with GBK into a String then you need to compare this value.
There was a problem hiding this comment.
As far as i am concerned, the parameter commandLine of CommandLineUtils.executeCommandLineAsCallable is used to create a Process object, which is a result of Runtime.getRuntime().execute(). This execute() method uses different encoding depending on different system.
Any idea of producing GBK bytes using CommandLineUtils.executeCommandLineAsCallable? Or i can modify the test to use system encoding rather than using GBK.
There was a problem hiding this comment.
By the way, this test will pass without this PR if system encoding is the same as the result of Charset.defaultCharset(). So the influence of this fix may not be very obvious.
There was a problem hiding this comment.
As far as i am concerned, the parameter commandLine of CommandLineUtils.executeCommandLineAsCallable is used to create a Process object, which is a result of Runtime.getRuntime().execute(). This execute() method uses different encoding depending on different system.
Why do you think so? It uses the same encoding as the surrounding Java process does. You cannot change this really on Windows, on Unix you can pass LC_ALL to the env.
Any idea of producing GBK bytes using CommandLineUtils.executeCommandLineAsCallable? Or i can modify the test to use system encoding rather than using GBK.
You have two options:
- Modify
file.encodingand set back in the finally block. Implies you read the output stream. I don't exactly know whether tests can run in parallel in the same JVM, this could break other tests. - Write a simple Java program, put it in
src/test/java, call the.classfile with Java from within the test. It should useSystem.outas a byte-oriented stream which will write bytes according to GBK. Read those with the consumer and check when normalized back to UTF-16.
There was a problem hiding this comment.
Write a simple Java program, put it in src/test/java, call the .class file with Java from within the test. It should use System.out as a byte-oriented stream which will write bytes according to GBK. Read those with the consumer and check when normalized back to UTF-16.
I tried producing gbk bytes with System.out.println(new String("金色传说".getBytes(), "GBK"). When comparing value in the cousumer, test passes on a windows-gbk platform but fails on a mac-utf-8 platform. Maybe it's not right to produce gbk bytes like that.
There was a problem hiding this comment.
That's wrong. You need to use System.out as an byte stream, not a char stream: byte[] bytes = "...".getBytes(encoding) then System.out.write(bytes).
|
Resolve #302 |
5 participants
add charset config