Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LANG-1752 seperated test cases of Strings from StringUtils #1279

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MSaifAsif
Copy link
Contributor

@MSaifAsif MSaifAsif commented Sep 26, 2024

Separated out test cases from StringUtils. Added more cases for null check, it seems that we are running into a NPE when we use the new Strings.CS.equals(null,null) method. See line where I created a test cases to highlight the failure. When using StringUtils.equals(null, null) it doesn't throw a NPE. IMO We should use the same functionality when using either of the Strings.CS or Strings.CI equals method

@@ -37,54 +34,22 @@ public class StringUtilsContainsTest extends AbstractLangTest {
public void testContains_Char() {
assertFalse(StringUtils.contains(null, ' '));
assertFalse(StringUtils.contains("", ' '));
assertFalse(StringUtils.contains("", null));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MSaifAsif
Do NOT delete existing tests please.

Copy link
Contributor Author

@MSaifAsif MSaifAsif Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I migrated it over here. Would you prefer that we keep the existing StringUtils test cases as well on top of the separation that I did ?
I haven't deleted any test case, they are just migrated over. I went through all the pre-existing test cases that were now showing deprecated api usage and migrated them over to the Strings related test cases. The test count should be the same as before, in fact should be more (since new test classes were made)

@MSaifAsif
Copy link
Contributor Author

Reverted back the existing test cases @garydgregory I thought about it and yes its too risky of a move to not keep them. Reverted them back

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants