ICU ticket #12590: Import unit tests from Android project into public ICU4J

ICU ticket #12590: Import unit tests from Android project into public ICU4J

[icu.sgill, 2016-06-23 21:22:06]

https://codereview.appspot.com/298570043/diff/1/main/tests/collate/src/com/ib...
File
main/tests/collate/src/com/ibm/icu/dev/test/collator/AlphabeticIndexTest.java
(right):

https://codereview.appspot.com/298570043/diff/1/main/tests/collate/src/com/ib...
main/tests/collate/src/com/ibm/icu/dev/test/collator/AlphabeticIndexTest.java:1086:
@Test
newline between methods

https://codereview.appspot.com/298570043/diff/1/main/tests/core/src/com/ibm/i...
File
main/tests/core/src/com/ibm/icu/dev/test/format/RelativeDateTimeFormatterTest.java
(right):

https://codereview.appspot.com/298570043/diff/1/main/tests/core/src/com/ibm/i...
main/tests/core/src/com/ibm/icu/dev/test/format/RelativeDateTimeFormatterTest.java:946:
public void testGetInstance() {
Questionable test that is testing whether two locales currently share the same
formatting data. If that data changes this test will fail. It needs to set the
default locale to Canada first. However, even then the test is just checking
whether the same data is loaded for the locale when it is default versus when it
is explicit. Seems irrelevant.

https://codereview.appspot.com/298570043/diff/1/main/tests/core/src/com/ibm/i...
File main/tests/core/src/com/ibm/icu/dev/test/lang/UnicodeSetTest.java (right):

https://codereview.appspot.com/298570043/diff/1/main/tests/core/src/com/ibm/i...
main/tests/core/src/com/ibm/icu/dev/test/lang/UnicodeSetTest.java:2721:
Set<String> test_set = Collections.emptySet();
This shouldn't be necessary. The assert line below it in the original test was
comparing the empt UnicodeSet against an empty list.

https://codereview.appspot.com/298570043/diff/1/main/tests/core/src/com/ibm/i...
File main/tests/core/src/com/ibm/icu/dev/test/util/CurrencyTest.java (right):

https://codereview.appspot.com/298570043/diff/1/main/tests/core/src/com/ibm/i...
main/tests/core/src/com/ibm/icu/dev/test/util/CurrencyTest.java:805: }
newline between tests

[andy.heninger, 2016-06-24 23:13:48]

These tests look plausible to me, modulo Stuart's comments.

https://codereview.appspot.com/298570043/diff/1/main/tests/core/src/com/ibm/i...
File
main/tests/core/src/com/ibm/icu/dev/test/format/RelativeDateTimeFormatterTest.java
(right):

https://codereview.appspot.com/298570043/diff/1/main/tests/core/src/com/ibm/i...
main/tests/core/src/com/ibm/icu/dev/test/format/RelativeDateTimeFormatterTest.java:946:
public void testGetInstance() {
On 2016/06/23 21:22:06, icu.sgill wrote:
> Questionable test that is testing whether two locales currently share the same
> formatting data. If that data changes this test will fail. It needs to set the
> default locale to Canada first. However, even then the test is just checking
> whether the same data is loaded for the locale when it is default versus when
it
> is explicit. Seems irrelevant.

I agree that this test looks funny. Since most of these Android tests were added
to improve code coverage, is it possible to find out what in ICU this one was
trying to hit?

[nrunge, 2016-06-30 23:15:45]

https://codereview.appspot.com/298570043/diff/1/main/tests/collate/src/com/ib...
File
main/tests/collate/src/com/ibm/icu/dev/test/collator/AlphabeticIndexTest.java
(right):

https://codereview.appspot.com/298570043/diff/1/main/tests/collate/src/com/ib...
main/tests/collate/src/com/ibm/icu/dev/test/collator/AlphabeticIndexTest.java:1086:
@Test
On 2016/06/23 21:22:06, icu.sgill wrote:
> newline between methods

Done.

https://codereview.appspot.com/298570043/diff/1/main/tests/core/src/com/ibm/i...
File
main/tests/core/src/com/ibm/icu/dev/test/format/RelativeDateTimeFormatterTest.java
(right):

https://codereview.appspot.com/298570043/diff/1/main/tests/core/src/com/ibm/i...
main/tests/core/src/com/ibm/icu/dev/test/format/RelativeDateTimeFormatterTest.java:946:
public void testGetInstance() {
On 2016/06/24 23:13:48, andy.heninger wrote:
> On 2016/06/23 21:22:06, icu.sgill wrote:
> > Questionable test that is testing whether two locales currently share the
same
> > formatting data. If that data changes this test will fail. It needs to set
the
> > default locale to Canada first. However, even then the test is just checking
> > whether the same data is loaded for the locale when it is default versus
when
> it
> > is explicit. Seems irrelevant.
> 
> I agree that this test looks funny. Since most of these Android tests were
added
> to improve code coverage, is it possible to find out what in ICU this one was
> trying to hit?

Notified Android about the concern, may take out this test from public ICU4J.

https://codereview.appspot.com/298570043/diff/1/main/tests/core/src/com/ibm/i...
File main/tests/core/src/com/ibm/icu/dev/test/lang/UnicodeSetTest.java (right):

https://codereview.appspot.com/298570043/diff/1/main/tests/core/src/com/ibm/i...
main/tests/core/src/com/ibm/icu/dev/test/lang/UnicodeSetTest.java:2721:
Set<String> test_set = Collections.emptySet();
On 2016/06/23 21:22:06, icu.sgill wrote:
> This shouldn't be necessary. The assert line below it in the original test was
> comparing the empt UnicodeSet against an empty list.

Unfortunately, it is, at least when compiling on Goobuntu.
Original error:
 error: no suitable method found for compareTo(List<Object>)
    [javac]         assertEquals("UnicodeSet not empty", 0,
UnicodeSet.EMPTY.compareTo(Collections.emptyList()));
    [javac]                                                                 ^

https://codereview.appspot.com/298570043/diff/1/main/tests/core/src/com/ibm/i...
File main/tests/core/src/com/ibm/icu/dev/test/util/CurrencyTest.java (right):

https://codereview.appspot.com/298570043/diff/1/main/tests/core/src/com/ibm/i...
main/tests/core/src/com/ibm/icu/dev/test/util/CurrencyTest.java:805: }
On 2016/06/23 21:22:06, icu.sgill wrote:
> newline between tests

Done.

[nrunge, 2016-07-19 22:22:25]

https://codereview.appspot.com/298570043/diff/1/main/tests/core/src/com/ibm/i...
File
main/tests/core/src/com/ibm/icu/dev/test/format/RelativeDateTimeFormatterTest.java
(right):

https://codereview.appspot.com/298570043/diff/1/main/tests/core/src/com/ibm/i...
main/tests/core/src/com/ibm/icu/dev/test/format/RelativeDateTimeFormatterTest.java:946:
public void testGetInstance() {
On 2016/06/30 23:15:45, nrunge wrote:
> On 2016/06/24 23:13:48, andy.heninger wrote:
> > On 2016/06/23 21:22:06, icu.sgill wrote:
> > > Questionable test that is testing whether two locales currently share the
> same
> > > formatting data. If that data changes this test will fail. It needs to set
> the
> > > default locale to Canada first. However, even then the test is just
checking
> > > whether the same data is loaded for the locale when it is default versus
> when
> > it
> > > is explicit. Seems irrelevant.
> > 
> > I agree that this test looks funny. Since most of these Android tests were
> added
> > to improve code coverage, is it possible to find out what in ICU this one
was
> > trying to hit?
> 
> Notified Android about the concern, may take out this test from public ICU4J.

Test deleted.

[nrunge, 2016-07-19 22:30:43]

code review changes r38988