Skip to content

chore: allow test-library static imports in test sources - #1282

Merged
nkuprins merged 5 commits into
apache:mainfrom
qiaolezi001:fix/fesod-1267-test-static-imports
Oct 30, 2026
Merged

nkuprins merged 5 commits into
apache:mainfrom
qiaolezi001:fix/fesod-1267-test-static-imports

Conversation

@qiaolezi001

@qiaolezi001 qiaolezi001 commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Purpose of the pull request

Fixes #1267.

The shared Checkstyle rules reject AssertJ's assertThat static import although AssertJ is already a test dependency. Allow named imports from common test libraries using Checkstyle's native allowlist.

What's changed?

  • Configure a single AvoidStaticImport module with excludes for JUnit Assertions/Assumptions, Mockito Mockito/ArgumentMatchers, and AssertJ Assertions, as requested in review. No XPath, duplicate rules or directory regex filters remain.
  • Keep AvoidStarImport unchanged: both static and regular wildcard imports remain forbidden, including imports from the allowlisted libraries. Other static imports remain forbidden.
  • Rely on the existing Maven test dependency scope to keep these libraries unavailable to production compilation. The allowlist itself is global; it does not enforce a test-source directory boundary.
  • Use assertThat(...).containsExactly(...) in ListUtilsTest alongside the existing JUnit assertions, preserving size, order and content coverage.
  • Update AGENTS.md and the configuration comment to describe the lint rule and Maven scope accurately.

Validation

Latest review update: only replaces the introductory XML comment block with a scope warning beside AvoidStaticImport. Parsed XML modules and property values are identical to 2171455. Six-module validate spotless:check and git diff --check passed for this update. The full test suite was not rerun for the comment-only change; the functional validation below was completed on 2171455, whose nine GitHub checks also passed.

  • 25 standalone Checkstyle 9.3 fixtures passed: named imports from all five owners are allowed; other static imports, similar owner names, and regular/static wildcard imports are rejected. Also covers multiline imports and Windows-style paths. Expected outcomes reflect the global allowlist.
  • Resolved fesod-common's actual compile/test classpaths with Maven and ran ten compiler checks: named imports from each of the five allowlisted owners fail on the production compile classpath and succeed on the test classpath.
  • ./mvnw -B validate spotless:check: all six modules passed.
  • ./mvnw -B clean package -pl fesod-common,fesod-shaded,fesod-sheet: 1,836 tests, 0 failures, 0 errors, 0 skips.
  • git diff --check passed for the changed files.

This revision was validated on Temurin JDK 25.0.5 using a workspace-local Maven cache and isolated temporary directories. No tests or assertions were modified or skipped for the review update. The multi-JDK matrix was not run locally; the new head must complete GitHub CI.

Checklist

  • I have read the Contributor Guide.
  • I have written the necessary doc or comment.
  • I have updated the necessary unit tests and all cases have passed.

Comment thread tools/checkstyle/checkstyle.xml Resolved
@bengbengbalabalabeng

Copy link
Copy Markdown
Contributor

Thanks for refactoring!

IMO having two separate rules with regex filters is still a bit overkill here. Since JUnit/Mockito/AssertJ are already scoped to test in Maven, src/main won't compile if someone tries to import them anyway.

Can we just keep it simple with a single AvoidStaticImport module and excludes. WDYT?

@bengbengbalabalabeng bengbengbalabalabeng changed the title fix: allow test-library static imports in test sources chore: allow test-library static imports in test sources Oct 29, 2026
Comment thread tools/checkstyle/checkstyle.xml Resolved

@nkuprins nkuprins left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thank you for your first contribution!

@nkuprins
nkuprins merged commit 126dde9 into apache:main Oct 30, 2026
9 checks passed
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.

[Enhancement] Allow static imports in test sources so AssertJ is usable

3 participants