Skip to content

Minor: Move StringUtil tests to opennlp-api, share test fixtures, reject null in StringUtil.isEmpty and isBlank - #1361

Merged
mawiesne merged 3 commits into
mainfrom
minor-dedup-test-support
Oct 3, 2026
Merged

mawiesne merged 3 commits into
mainfrom
minor-dedup-test-support

Conversation

@rzo1

@rzo1 rzo1 commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Moves StringUtilTest from opennlp-tools to opennlp-api and publishes an opennlp-api test-jar with shared Locale and WhitespaceMode test extensions, which replace copies in 15 test classes. StringUtil.isEmpty and isBlank now reject null with IllegalArgumentException like their siblings; before, both threw a NullPointerException, contrary to the isEmpty Javadoc.

StringUtilTest moves from opennlp-tools to opennlp-api/src/test.
StringUtil.isBlank rejects null with IllegalArgumentException.
opennlp-api publishes a test-jar with RestoreDefaultLocale,
ResetWhitespaceMode and PinWhitespaceMode, used by 15 test classes.
@rzo1 rzo1 added this to the 3.0.0 milestone Oct 2, 2026
isBlank now rejects null with IllegalArgumentException, isEmpty next to it
still throws NullPointerException and the Javadoc claims it returns true for
null. The changed test fails on the current code:

  StringUtilTest.testIsEmptyWithNullString
  Unexpected exception type thrown, expected: IllegalArgumentException
  but was: NullPointerException

Signed-off-by: Kristian Rickert <kristian@apache.org>
… the test-jar

isEmpty rejects null with IllegalArgumentException and documents the
parameter, the return value and the exception; the old Javadoc said it
returned true for null, which it never did. The opennlp-api test-jar
dependency in the formats and cli poms names ${project.groupId} like the
other opennlp dependencies in those files.

Signed-off-by: Kristian Rickert <kristian@apache.org>
@krickert

krickert commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Another small edit: we should throw IAE instead of NPE, so fixed it and made tests to prove the fix.

Fixed demonstrated with the red/green commits:

  • isBlank(null) now throws IllegalArgumentException, but isEmpty right next to it still produced an NPE, and the Javadoc promised true for null, which was not the case. isEmpty is aligned the same way, Javadoc corrected.
  • The opennlp-api test-jar dependency in the formats and cli poms had org.apache.opennlp written out; the other opennlp dependencies in those files use ${project.groupId}. Same here now.

Are you ok if we change the title to: "Move StringUtil tests to opennlp-api, share test fixtures, reject null in StringUtil.isEmpty and isBlank"?

Since the null handling of isEmpty and isBlank is a public API change under a test-move title, and titles end up in release notes. Either that or a line to that effect in the description.

Original solution description by AI, edited and hand validated by me:

The move keeps all coverage: 73 test methods and 285 executions in StringUtilTest before (opennlp-tools) and after (opennlp-api); test and assertion counts of the 15 consumer classes unchanged; the shared Locale and WhitespaceMode fixtures restore the same state the removed per-class copies did, and SentenceDetectorMESpanMappingTest and SourceForgeModelEval remain on LEGACY. The test-jar wiring resolves in the reactor and opennlp-eval-tests test-compiles. Merges clean with #1348, git follows the rename and the splitOnWhitespace tests from that PR end up in the moved class. api, cli, formats, ml-maxent, runtime, morfologik and tools pass locally.

@rzo1 rzo1 changed the title Minor: Move StringUtil tests to opennlp-api and share test fixtures Minor: Move StringUtil tests to opennlp-api, share test fixtures, reject null in StringUtil.isEmpty and isBlank Oct 2, 2026
@rzo1

rzo1 commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Thx for review. Updated the description and title.

@mawiesne mawiesne left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Happy +1

@mawiesne
mawiesne merged commit 773f107 into main Oct 3, 2026
10 checks passed
@mawiesne
mawiesne deleted the minor-dedup-test-support branch October 3, 2026 09:48
rzo1 added a commit that referenced this pull request Oct 3, 2026
…entChecks move (#1362)

#1353 removed the private StringUtil.requireNonNullArg while #1361 added
two callers of it, so opennlp-api no longer compiles on main. Both now
call ArgumentChecks.requireNonNullArg.
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.

3 participants