garydgregory commented on code in PR #732:
URL: https://github.com/apache/commons-text/pull/732#discussion_r2612105445
##########
src/test/java/org/apache/commons/text/translate/CharSequenceTranslatorTest.java:
##########
@@ -46,8 +48,20 @@ void testWith() throws IOException {
final CharSequenceTranslator charSequenceTranslatorThree = new
TestCharSequenceTranslator();
final CharSequenceTranslator aggregatedTranslator =
charSequenceTranslatorOne.with(charSequenceTranslatorTwo,
charSequenceTranslatorThree);
aggregatedTranslator.translate("", 0, null);
- assertTrue(aggregatedTranslator instanceof AggregateTranslator);
+ assertInstanceOf(AggregateTranslator.class, aggregatedTranslator);
Review Comment:
Good one :)
##########
src/test/java/org/apache/commons/text/lookup/StringLookupFactoryTest.java:
##########
@@ -257,6 +257,16 @@ void testSingletons() {
assertSame(XmlEncoderStringLookup.INSTANCE,
stringLookupFactory.xmlEncoderStringLookup());
}
+ /**
+ * Tests that we return the singleton.
+ */
+ @Deprecated
Review Comment:
We don't use the `Deprecated` annotations on tests.
##########
src/test/java/org/apache/commons/text/StringSubstitutorTest.java:
##########
@@ -1085,4 +1085,11 @@ void testSubstitutePreserveEscape() throws IOException {
assertEqualsCharSeq("value $${escaped}", replace(sub, org));
}
+ @Test
+ void testToString() {
+ final StringSubstitutor s = new StringSubstitutor(null, "prefix",
"suffix");
+ String str = s.toString();
+ assertTrue(str.contains("\"prefix\""),
Review Comment:
All on 1 line please.
##########
src/test/java/org/apache/commons/text/similarity/LevenshteinResultsTest.java:
##########
@@ -62,4 +71,12 @@ void testEqualsWithNull() {
assertFalse(levenshteinResults.equals(null));
}
+ @Test
+ void testEqualsWithDifferentObject() {
+ final Integer integer = -647;
+ final LevenshteinResults levenshteinResults = new
LevenshteinResults(integer, null, null, integer);
+ //noinspection EqualsBetweenInconvertibleTypes
Review Comment:
Remove this metadata comment.
##########
src/test/java/org/apache/commons/text/WordUtilsTest.java:
##########
@@ -321,6 +323,32 @@ void testInitialsSurrogatePairs() {
new char[] { '\uD800', '\uDF14', '\uD800', '\uDF18' }));
}
+ @Deprecated
Review Comment:
Remove `@Deprecated`.
##########
src/test/java/org/apache/commons/text/similarity/IntersectionResultTest.java:
##########
@@ -45,6 +45,15 @@ void testEquals() {
for (int j = 0; j < results.length; j++) {
Assertions.assertEquals(results[i].equals(results[j]), i == j);
}
+
+ // IntelliJ would like to optimize this, but it would make
+ // the test useless as assertNotEquals() handles null itself
+ //noinspection ConstantValue,SimplifiableAssertion
+ Assertions.assertFalse(results[i].equals(null),
Review Comment:
No newspaper column-style formatting, please. See our checkstyle.xml file,
which allows line lengths up to 160.
##########
src/test/java/org/apache/commons/text/similarity/LongestCommonSubsequenceTest.java:
##########
@@ -106,6 +108,8 @@ void testLongestCommonSubsequence() {
assertEquals("", subject.longestCommonSubsequence("", ""));
assertEquals("", subject.longestCommonSubsequence("left", ""));
assertEquals("", subject.longestCommonSubsequence("", "right"));
+ assertEquals("", subject.longestCommonSubsequence("l", "a"));
+ assertEquals("", subject.longestCommonSubsequence("left", "a"));
assertEquals("fog", subject.longestCommonSubsequence("frog", "fog"));
assertEquals("", subject.longestCommonSubsequence("fly", "ant"));
Review Comment:
This doesn't improve the coverage beyond what we already have: 92%. What
does this test that the current test doesn't? Unless you can explain it, please
remove these changes. TY.
##########
src/test/java/org/apache/commons/text/lookup/DateStringLookupTest.java:
##########
@@ -42,9 +43,16 @@ void testBadFormat() {
@Test
void testDefault() throws ParseException {
- final String formatted = DateStringLookup.INSTANCE.apply(null);
- DateFormat.getInstance().parse(formatted); // throws ParseException
+ Locale prev = Locale.getDefault();
+ try {
+ // ensure that date-parser uses English/American presets
Review Comment:
Why is this important for this test?
That's not how you set with Locale's anyway, this component depends on JUnit
Pioneer for this type of stuff (see `@DefaultLocale`)
I think you can remove this test as this class is already at 100% coverage:
<img width="960" height="205" alt="Image"
src="https://github.com/user-attachments/assets/01dcfa27-24d9-491c-a7f8-4159caf7e806";
/>
##########
src/test/java/org/apache/commons/text/lookup/StringLookupFactoryTest.java:
##########
@@ -257,6 +257,16 @@ void testSingletons() {
assertSame(XmlEncoderStringLookup.INSTANCE,
stringLookupFactory.xmlEncoderStringLookup());
}
+ /**
+ * Tests that we return the singleton.
+ */
+ @Deprecated
+ @Test
+ void testDeprecatedSingletons() {
+ final StringLookupFactory stringLookupFactory =
StringLookupFactory.INSTANCE;
Review Comment:
This local variable is not needed.
##########
src/test/java/org/apache/commons/text/similarity/LongestCommonSubsequenceTest.java:
##########
@@ -136,6 +140,7 @@ void testLongestCommonSubsequenceApply() {
}
@Test
+ @Deprecated
Review Comment:
Could you remove the annotation?
##########
src/test/java/org/apache/commons/text/WordUtilsTest.java:
##########
@@ -321,6 +323,32 @@ void testInitialsSurrogatePairs() {
new char[] { '\uD800', '\uDF14', '\uD800', '\uDF18' }));
}
+ @Deprecated
+ @Test
+ void testIsDelimiter() {
+ assertFalse(WordUtils.isDelimiter('.', null));
+ assertTrue(WordUtils.isDelimiter(' ', null));
+
+ assertFalse(WordUtils.isDelimiter(' ', new char[] { '.' }));
+ assertTrue(WordUtils.isDelimiter('.', new char[] { '.' }));
+
+ assertFalse(WordUtils.isDelimiter(' ', new char[] { '.', '_', 'a' }));
+ assertTrue(WordUtils.isDelimiter('.', new char[] { '.', '_', 'a', '.'
}));
+ }
+
+ @Deprecated
Review Comment:
Remove `@Deprecated`.
##########
src/test/java/org/apache/commons/text/translate/OctalUnescaperTest.java:
##########
@@ -79,4 +83,33 @@ void testBetween() {
assertEquals("\\999", result, "Failed to ignore an out of range octal
character via the between method");
}
+ @Test
+ void testInvalid() {
+ final OctalUnescaper oue = new OctalUnescaper();
+
+ String input = "\\4a";
Review Comment:
Use final where you can.
##########
src/test/java/org/apache/commons/text/similarity/IntersectionResultTest.java:
##########
@@ -45,6 +45,15 @@ void testEquals() {
for (int j = 0; j < results.length; j++) {
Assertions.assertEquals(results[i].equals(results[j]), i == j);
}
+
+ // IntelliJ would like to optimize this, but it would make
+ // the test useless as assertNotEquals() handles null itself
+ //noinspection ConstantValue,SimplifiableAssertion
+ Assertions.assertFalse(results[i].equals(null),
+ "Should not be Equal to null");
+ //noinspection AssertBetweenInconvertibleTypes
Review Comment:
Remove this metadata comment.
##########
src/test/java/org/apache/commons/text/translate/OctalUnescaperTest.java:
##########
@@ -79,4 +83,33 @@ void testBetween() {
assertEquals("\\999", result, "Failed to ignore an out of range octal
character via the between method");
}
+ @Test
+ void testInvalid() {
+ final OctalUnescaper oue = new OctalUnescaper();
+
Review Comment:
Remove blank line. If the code needs "phrasing", explain it in a // comment.
##########
src/test/java/org/apache/commons/text/translate/UnicodeUnescaperTest.java:
##########
@@ -49,4 +49,11 @@ void testUuuuu() {
final String result = escaper.translate(input);
assertEquals("G", result, "Failed to unescape Unicode characters with
many 'u' characters");
}
+
+ @Test
+ void testTooShort() {
+ final UnicodeUnescaper escaper = new UnicodeUnescaper();
Review Comment:
Please inline these single-use locals.
##########
src/test/java/org/apache/commons/text/similarity/LongestCommonSubsequenceTest.java:
##########
@@ -89,6 +89,8 @@ void testLogestCommonSubsequence() {
assertEquals("", subject.logestCommonSubsequence("", ""));
assertEquals("", subject.logestCommonSubsequence("left", ""));
assertEquals("", subject.logestCommonSubsequence("", "right"));
+ assertEquals("", subject.logestCommonSubsequence("l", "a"));
Review Comment:
This doesn't improve the coverage beyond what we already have: 92%. What
does this test that the current test doesn't? Unless you can explain it, please
remove these changes. TY.
##########
src/test/java/org/apache/commons/text/lookup/StringLookupFactoryTest.java:
##########
@@ -279,4 +289,9 @@ void testXmlStringLookupExternalEntityOn() {
assertEquals(XmlStringLookupTest.DATA,
StringLookupFactory.INSTANCE.xmlStringLookup(XmlStringLookupTest.EMPTY_MAP).apply(key).trim());
}
+ @Test
+ void testClear() {
+ // this will clear out the global cache in ConstantStringLookup
+ StringLookupFactory.clear();
Review Comment:
Is there anything you can assert here? Otherwise, this test only shows the
API doen't thrown an unchecked exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
