ICU ticket #13253: Parameterizes data driven test, replaces encoded characters in test data with Un…

ICU ticket #13253: Parameterizes data driven test, replaces encoded characters in test data with Un…

[markus.icu, 2017-08-08 23:13:41]

https://codereview.appspot.com/322500043/diff/1/main/tests/core/src/com/ibm/i...
File
main/tests/core/src/com/ibm/icu/dev/test/shaping/DataDrivenArabicShapingRegTest.java
(right):

https://codereview.appspot.com/322500043/diff/1/main/tests/core/src/com/ibm/i...
main/tests/core/src/com/ibm/icu/dev/test/shaping/DataDrivenArabicShapingRegTest.java:62:
@Parameterized.Parameters
funky indentation

Try to avoid the Eclipse auto-formatter...

https://codereview.appspot.com/322500043/diff/1/main/tests/core/src/com/ibm/i...
main/tests/core/src/com/ibm/icu/dev/test/shaping/DataDrivenArabicShapingRegTest.java:64:
String lamAlefSpecialVLTR = " نآلا نأَلا ناٰلا نآٓلا نإٕلا نآٔلا ﻼع";
I trust you with the parameterization, but I think the unescaping is mostly a
mistake. With RTL letters and letter shaping and deliberate differences between
presentation forms and normal letters, the intent of the test data becomes
completely obscure.

Replacing \u0020 with a simple space is fine.

Replacing the pairs of escaped surrogates with the actual characters (math
digits) is probably ok too, although on my machine in the review font I get
tofus.

The Arabic letters and their presentation forms should remain escaped.

The Arabic digits (two sets 066x & 06Fx) should remain escaped as well, since
most of them look alike across the sets.

[nrunge, 2017-08-09 19:36:57]

Changes address review comments

[nrunge, 2017-08-09 19:43:57]

On 2017/08/08 23:13:41, markus.icu wrote:
>
https://codereview.appspot.com/322500043/diff/1/main/tests/core/src/com/ibm/i...
> File
>
main/tests/core/src/com/ibm/icu/dev/test/shaping/DataDrivenArabicShapingRegTest.java
> (right):
> 
>
https://codereview.appspot.com/322500043/diff/1/main/tests/core/src/com/ibm/i...
>
main/tests/core/src/com/ibm/icu/dev/test/shaping/DataDrivenArabicShapingRegTest.java:62:
> @Parameterized.Parameters
> funky indentation
> 
> Try to avoid the Eclipse auto-formatter...
> 
>
https://codereview.appspot.com/322500043/diff/1/main/tests/core/src/com/ibm/i...
>
main/tests/core/src/com/ibm/icu/dev/test/shaping/DataDrivenArabicShapingRegTest.java:64:
> String lamAlefSpecialVLTR = " نآلا نأَلا ناٰلا نآٓلا نإٕلا نآٔلا ﻼع";
> I trust you with the parameterization, but I think the unescaping is mostly a
> mistake. With RTL letters and letter shaping and deliberate differences
between
> presentation forms and normal letters, the intent of the test data becomes
> completely obscure.
> 
> Replacing \u0020 with a simple space is fine.
Put back the '\uxxxx' encoding. Retained '\u0020' to preserve equal row length
of
the data blocks, which allows.

> 
> Replacing the pairs of escaped surrogates with the actual characters (math
> digits) is probably ok too, although on my machine in the review font I get
> tofus.
My browser shows tofu, too. Eclipse gets it right, with a few exceptions.
Reversed back to escaped form.

> 
> The Arabic letters and their presentation forms should remain escaped.
> 
> The Arabic digits (two sets 066x & 06Fx) should remain escaped as well, since
> most of them look alike across the sets.

[markus.icu, 2017-08-17 22:02:10]

LGTM