[GitHub] [commons-lang] garydgregory commented on a change in pull request #682: [LANG-1631] - Check if Character is defined

[GitBox]

garydgregory commented on a change in pull request #682:
URL: https://github.com/apache/commons-lang/pull/682#discussion_r547325346



##########
File path: 
src/test/java/org/apache/commons/lang3/StringUtilsEqualsIndexOfTest.java
##########
@@ -506,42 +510,57 @@ public void testIndexOfIgnoreCase_StringInt() {
         assertEquals(4, StringUtils.indexOfIgnoreCase("aabaabaa", "AB", 2));
         assertEquals(4, StringUtils.indexOfIgnoreCase("aabaabaa", "AB", 3));
         assertEquals(4, StringUtils.indexOfIgnoreCase("aabaabaa", "AB", 4));
-        assertEquals(-1, StringUtils.indexOfIgnoreCase("aabaabaa", "AB", 5));
-        assertEquals(-1, StringUtils.indexOfIgnoreCase("aabaabaa", "AB", 6));
-        assertEquals(-1, StringUtils.indexOfIgnoreCase("aabaabaa", "AB", 7));
-        assertEquals(-1, StringUtils.indexOfIgnoreCase("aabaabaa", "AB", 8));
+        assertEquals(NOT_FOUND, StringUtils.indexOfIgnoreCase("aabaabaa", 
"AB", 5));
+        assertEquals(NOT_FOUND, StringUtils.indexOfIgnoreCase("aabaabaa", 
"AB", 6));
+        assertEquals(NOT_FOUND, StringUtils.indexOfIgnoreCase("aabaabaa", 
"AB", 7));
+        assertEquals(NOT_FOUND, StringUtils.indexOfIgnoreCase("aabaabaa", 
"AB", 8));
         assertEquals(1, StringUtils.indexOfIgnoreCase("aab", "AB", 1));
         assertEquals(5, StringUtils.indexOfIgnoreCase("aabaabaa", "", 5));
-        assertEquals(-1, StringUtils.indexOfIgnoreCase("ab", "AAB", 0));
-        assertEquals(-1, StringUtils.indexOfIgnoreCase("aab", "AAB", 1));
-        assertEquals(-1, StringUtils.indexOfIgnoreCase("abc", "", 9));
+        assertEquals(NOT_FOUND, StringUtils.indexOfIgnoreCase("ab", "AAB", 0));
+        assertEquals(NOT_FOUND, StringUtils.indexOfIgnoreCase("aab", "AAB", 
1));
+        assertEquals(NOT_FOUND, StringUtils.indexOfIgnoreCase("abc", "", 9));
     }
 
     @Test
     public void testLastIndexOf_char() {
-        assertEquals(-1, StringUtils.lastIndexOf(null, ' '));
-        assertEquals(-1, StringUtils.lastIndexOf("", ' '));
+        assertEquals(NOT_FOUND, StringUtils.lastIndexOf(null, ' '));
+        assertEquals(NOT_FOUND, StringUtils.lastIndexOf("", ' '));
         assertEquals(7, StringUtils.lastIndexOf("aabaabaa", 'a'));
         assertEquals(5, StringUtils.lastIndexOf("aabaabaa", 'b'));
 
         assertEquals(5, StringUtils.lastIndexOf(new StringBuilder("aabaabaa"), 
'b'));
+        char[] charArray = new char[9];
+        charArray[0] = '[';
+        charArray[1] = '&';
+        charArray[7] = '{';
+        charArray[3] = '.';
+        charArray[4] = 'c';
+        charArray[5] = '.';
+        charArray[6] = '0';
+        charArray[7] = 'r';
+        charArray[8] = 'o';
+        CharBuffer charBuffer = CharBuffer.wrap(charArray);
+        assertEquals(NOT_FOUND, StringUtils.lastIndexOf (charBuffer, (-1738), 
982));

Review comment:
       Hi @arturobernalg 
   
   Thank you for your PR. 
   
   PS: It's harder to deal with a PR like this because it mixes a change with 
some clean ups (-1 -> NOT_FOUND) so I am not sure where the new test is without 
trolling through all the noise :-(
   
   Why is the number in parentheses? If this is a test for the actual bug fix, 
then you should add a sanity check to validates that the char is indeed bogus.
   
   TY.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org