[15] RFR: 8241311: Move some charset mapping tests from closed to open
Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8241311 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8241311/webrev.04/ This is simply to move some test cases that have been in closed repository into open repository. Naoto
Hello Naoto. I'm not reviewer, but I have a concern about following code on test/jdk/sun/nio/cs/mapping/TestConv.java ====== 98 } catch (Exception ex) { 99 System.out.println("Exception thrown while testing " + encoding); 100 ex.printStackTrace(); 101 return; 102 } ====== 3 stack trace (java.io.UnsupportedEncodingException) were displayed against following encodings: MS50221_0208, MS50221_0212, MS932_0208 I think only UnsupportedEncodingException should be caught. The other exception should be handled as error. What do you think ? Thanks, Ichiroh Takiguchi On 2020-03-21 01:21, naoto.sato@oracle.com wrote:
Hello,
Please review the fix to the following issue:
https://bugs.openjdk.java.net/browse/JDK-8241311
The proposed changeset is located at:
https://cr.openjdk.java.net/~naoto/8241311/webrev.04/
This is simply to move some test cases that have been in closed repository into open repository.
Naoto
Hi Takiguchi-san, On 3/23/20 5:48 AM, Ichiroh Takiguchi wrote:
Hello Naoto.
I'm not reviewer, but I have a concern about following code on test/jdk/sun/nio/cs/mapping/TestConv.java ====== 98 } catch (Exception ex) { 99 System.out.println("Exception thrown while testing " + encoding); 100 ex.printStackTrace(); 101 return; 102 } ======
3 stack trace (java.io.UnsupportedEncodingException) were displayed against following encodings: MS50221_0208, MS50221_0212, MS932_0208
I think only UnsupportedEncodingException should be caught. The other exception should be handled as error.
What do you think ?
Good catch. I believe the test logic that assumes the file name is the charset name is simply wrong. I added the check whether the input charset name is supported or not, and only do the test if the charset is supported: http://cr.openjdk.java.net/~naoto/8241311/webrev.05/ Naoto
Thanks, Ichiroh Takiguchi
On 2020-03-21 01:21, naoto.sato@oracle.com wrote:
Hello,
Please review the fix to the following issue:
https://bugs.openjdk.java.net/browse/JDK-8241311
The proposed changeset is located at:
https://cr.openjdk.java.net/~naoto/8241311/webrev.04/
This is simply to move some test cases that have been in closed repository into open repository.
Naoto
Hello Naoto. I tested webrev.06 code. It's fine, thanks. I'm interested in about @module for these testcases. I think webrev.04 code worked via jtreg. I could not see any warning. At this case, @module is required ? Thanks, Ichiroh Takiguchi On 2020-03-24 10:06, naoto.sato@oracle.com wrote:
Hi Takiguchi-san,
On 3/23/20 5:48 AM, Ichiroh Takiguchi wrote:
Hello Naoto.
I'm not reviewer, but I have a concern about following code on test/jdk/sun/nio/cs/mapping/TestConv.java ====== 98 } catch (Exception ex) { 99 System.out.println("Exception thrown while testing " + encoding); 100 ex.printStackTrace(); 101 return; 102 } ======
3 stack trace (java.io.UnsupportedEncodingException) were displayed against following encodings: MS50221_0208, MS50221_0212, MS932_0208
I think only UnsupportedEncodingException should be caught. The other exception should be handled as error.
What do you think ?
Good catch. I believe the test logic that assumes the file name is the charset name is simply wrong. I added the check whether the input charset name is supported or not, and only do the test if the charset is supported:
http://cr.openjdk.java.net/~naoto/8241311/webrev.05/
Naoto
Thanks, Ichiroh Takiguchi
On 2020-03-21 01:21, naoto.sato@oracle.com wrote:
Hello,
Please review the fix to the following issue:
https://bugs.openjdk.java.net/browse/JDK-8241311
The proposed changeset is located at:
https://cr.openjdk.java.net/~naoto/8241311/webrev.04/
This is simply to move some test cases that have been in closed repository into open repository.
Naoto
On 3/24/20 9:24 AM, Ichiroh Takiguchi wrote:
Hello Naoto.
I tested webrev.06 code. It's fine, thanks.
I'm interested in about @module for these testcases. I think webrev.04 code worked via jtreg. I could not see any warning. At this case, @module is required ?
Yes. The tag lets jtreg not run the test if jdk.charsets module is not present in the test JDK. Otherwise, it may give you the false negative result. Naoto
Thanks, Ichiroh Takiguchi
On 2020-03-24 10:06, naoto.sato@oracle.com wrote:
Hi Takiguchi-san,
On 3/23/20 5:48 AM, Ichiroh Takiguchi wrote:
Hello Naoto.
I'm not reviewer, but I have a concern about following code on test/jdk/sun/nio/cs/mapping/TestConv.java ====== 98 } catch (Exception ex) { 99 System.out.println("Exception thrown while testing " + encoding); 100 ex.printStackTrace(); 101 return; 102 } ======
3 stack trace (java.io.UnsupportedEncodingException) were displayed against following encodings: MS50221_0208, MS50221_0212, MS932_0208
I think only UnsupportedEncodingException should be caught. The other exception should be handled as error.
What do you think ?
Good catch. I believe the test logic that assumes the file name is the charset name is simply wrong. I added the check whether the input charset name is supported or not, and only do the test if the charset is supported:
http://cr.openjdk.java.net/~naoto/8241311/webrev.05/
Naoto
Thanks, Ichiroh Takiguchi
On 2020-03-21 01:21, naoto.sato@oracle.com wrote:
Hello,
Please review the fix to the following issue:
https://bugs.openjdk.java.net/browse/JDK-8241311
The proposed changeset is located at:
https://cr.openjdk.java.net/~naoto/8241311/webrev.04/
This is simply to move some test cases that have been in closed repository into open repository.
Naoto
Hi, Naoto CoderTest.java TestConv.java Should they also include @modules jdk.charsets ? Thanks, Amy On 3/21/20 12:21 AM, naoto.sato@oracle.com wrote:
Hello,
Please review the fix to the following issue:
https://bugs.openjdk.java.net/browse/JDK-8241311
The proposed changeset is located at:
https://cr.openjdk.java.net/~naoto/8241311/webrev.04/
This is simply to move some test cases that have been in closed repository into open repository.
Naoto
Hi Amy, Good point. Updated. https://cr.openjdk.java.net/~naoto/8241311/webrev.06/ Naoto On 3/23/20 7:43 PM, Amy Lu wrote:
Hi, Naoto
CoderTest.java TestConv.java Should they also include @modules jdk.charsets ?
Thanks, Amy
On 3/21/20 12:21 AM, naoto.sato@oracle.com wrote:
Hello,
Please review the fix to the following issue:
https://bugs.openjdk.java.net/browse/JDK-8241311
The proposed changeset is located at:
https://cr.openjdk.java.net/~naoto/8241311/webrev.04/
This is simply to move some test cases that have been in closed repository into open repository.
Naoto
Thank you Naoto for the quick update. Just more findings ...(sorry for not sending earlier) CoderTest.java Looks like this test has already been migrated (with enhancement) to the 'open' [1] in JDK-8186801 [2] [3]. I'm not sure whether the data files that used by TestCharsetMapping.java can be leveraged by other two tests, but worth checking. Thanks, Amy [1] http://hg.openjdk.java.net/jdk/jdk/file/tip/test/jdk/sun/nio/cs/TestCharsetM... [2] https://bugs.openjdk.java.net/browse/JDK-8186801 [3] https://mail.openjdk.java.net/pipermail/nio-dev/2017-August/004427.html On 3/24/20 12:19 PM, naoto.sato@oracle.com wrote:
Hi Amy,
Good point. Updated.
https://cr.openjdk.java.net/~naoto/8241311/webrev.06/
Naoto
On 3/23/20 7:43 PM, Amy Lu wrote:
Hi, Naoto
CoderTest.java TestConv.java Should they also include @modules jdk.charsets ?
Thanks, Amy
On 3/21/20 12:21 AM, naoto.sato@oracle.com wrote:
Hello,
Please review the fix to the following issue:
https://bugs.openjdk.java.net/browse/JDK-8241311
The proposed changeset is located at:
https://cr.openjdk.java.net/~naoto/8241311/webrev.04/
This is simply to move some test cases that have been in closed repository into open repository.
Naoto
Thanks again. I will look into it and come up with better migration of those tests. Naoto On 3/23/20 9:45 PM, Amy Lu wrote:
Thank you Naoto for the quick update.
Just more findings ...(sorry for not sending earlier)
CoderTest.java Looks like this test has already been migrated (with enhancement) to the 'open' [1] in JDK-8186801 [2] [3].
I'm not sure whether the data files that used by TestCharsetMapping.java can be leveraged by other two tests, but worth checking.
Thanks, Amy
[1] http://hg.openjdk.java.net/jdk/jdk/file/tip/test/jdk/sun/nio/cs/TestCharsetM...
[2] https://bugs.openjdk.java.net/browse/JDK-8186801 [3] https://mail.openjdk.java.net/pipermail/nio-dev/2017-August/004427.html
On 3/24/20 12:19 PM, naoto.sato@oracle.com wrote:
Hi Amy,
Good point. Updated.
https://cr.openjdk.java.net/~naoto/8241311/webrev.06/
Naoto
On 3/23/20 7:43 PM, Amy Lu wrote:
Hi, Naoto
CoderTest.java TestConv.java Should they also include @modules jdk.charsets ?
Thanks, Amy
On 3/21/20 12:21 AM, naoto.sato@oracle.com wrote:
Hello,
Please review the fix to the following issue:
https://bugs.openjdk.java.net/browse/JDK-8241311
The proposed changeset is located at:
https://cr.openjdk.java.net/~naoto/8241311/webrev.04/
This is simply to move some test cases that have been in closed repository into open repository.
Naoto
Hi Amy, I ended up the fix pretty much as of ver 06, as those data file formats are different. It'd require some amount of refactoring to those other two tests, and CoderTest.java is a dependence to ConverterTest.java, so I left them as-is. One minor change from 06 is to add @modules jdk.charsets to TestCharsetMapping.java. http://cr.openjdk.java.net/~naoto/8241311/webrev.07/ Naoto On 3/23/20 9:45 PM, Amy Lu wrote:
Thank you Naoto for the quick update.
Just more findings ...(sorry for not sending earlier)
CoderTest.java Looks like this test has already been migrated (with enhancement) to the 'open' [1] in JDK-8186801 [2] [3].
I'm not sure whether the data files that used by TestCharsetMapping.java can be leveraged by other two tests, but worth checking.
Thanks, Amy
[1] http://hg.openjdk.java.net/jdk/jdk/file/tip/test/jdk/sun/nio/cs/TestCharsetM...
[2] https://bugs.openjdk.java.net/browse/JDK-8186801 [3] https://mail.openjdk.java.net/pipermail/nio-dev/2017-August/004427.html
On 3/24/20 12:19 PM, naoto.sato@oracle.com wrote:
Hi Amy,
Good point. Updated.
https://cr.openjdk.java.net/~naoto/8241311/webrev.06/
Naoto
On 3/23/20 7:43 PM, Amy Lu wrote:
Hi, Naoto
CoderTest.java TestConv.java Should they also include @modules jdk.charsets ?
Thanks, Amy
On 3/21/20 12:21 AM, naoto.sato@oracle.com wrote:
Hello,
Please review the fix to the following issue:
https://bugs.openjdk.java.net/browse/JDK-8241311
The proposed changeset is located at:
https://cr.openjdk.java.net/~naoto/8241311/webrev.04/
This is simply to move some test cases that have been in closed repository into open repository.
Naoto
That's fine then. Thank you for looking into this. Thanks, Amy On 3/25/20 4:37 AM, naoto.sato@oracle.com wrote:
Hi Amy,
I ended up the fix pretty much as of ver 06, as those data file formats are different. It'd require some amount of refactoring to those other two tests, and CoderTest.java is a dependence to ConverterTest.java, so I left them as-is.
One minor change from 06 is to add @modules jdk.charsets to TestCharsetMapping.java.
http://cr.openjdk.java.net/~naoto/8241311/webrev.07/
Naoto
On 3/23/20 9:45 PM, Amy Lu wrote:
Thank you Naoto for the quick update.
Just more findings ...(sorry for not sending earlier)
CoderTest.java Looks like this test has already been migrated (with enhancement) to the 'open' [1] in JDK-8186801 [2] [3].
I'm not sure whether the data files that used by TestCharsetMapping.java can be leveraged by other two tests, but worth checking.
Thanks, Amy
[1] http://hg.openjdk.java.net/jdk/jdk/file/tip/test/jdk/sun/nio/cs/TestCharsetM...
[2] https://bugs.openjdk.java.net/browse/JDK-8186801 [3] https://mail.openjdk.java.net/pipermail/nio-dev/2017-August/004427.html
On 3/24/20 12:19 PM, naoto.sato@oracle.com wrote:
Hi Amy,
Good point. Updated.
https://cr.openjdk.java.net/~naoto/8241311/webrev.06/
Naoto
On 3/23/20 7:43 PM, Amy Lu wrote:
Hi, Naoto
CoderTest.java TestConv.java Should they also include @modules jdk.charsets ?
Thanks, Amy
On 3/21/20 12:21 AM, naoto.sato@oracle.com wrote:
Hello,
Please review the fix to the following issue:
https://bugs.openjdk.java.net/browse/JDK-8241311
The proposed changeset is located at:
https://cr.openjdk.java.net/~naoto/8241311/webrev.04/
This is simply to move some test cases that have been in closed repository into open repository.
Naoto
participants (4)
-
Amy Lu
-
Ichiroh Takiguchi
-
Iris Clark
-
naoto.sato＠oracle.com