Skip to content

Issue #17910: Remove suppressions for the ImportControlTest rule#17911

Merged
romani merged 1 commit into
checkstyle:masterfrom
mureinik:17910-import-control-test-supression
Oct 13, 2025
Merged

Issue #17910: Remove suppressions for the ImportControlTest rule#17911
romani merged 1 commit into
checkstyle:masterfrom
mureinik:17910-import-control-test-supression

Conversation

@mureinik

@mureinik mureinik commented Oct 11, 2025

Copy link
Copy Markdown
Contributor

The ImprotControlTest rule should govern which imports aren't (and are) allowed in test files. However, for many tests, this rule is suppressed, so the configuration is not adhered to, leading to classes that aren't supposed to be used in tests.

This PR aims to address this issue.
It removes these suppressions, and where needed, replaces them with finer-grained "allow" entries in import-control-test.xml.

This PR addresses the following concerns:

Usage of reflection

  • As per the guidelines of All reflection-based test should be reviewed and changed (or whitelisted) #11123, tests that use reflection to either introspect on the internal (private) state of an object, or execute some method/constructor that should be private, were rewritten to use the utilities from TestUtil that provide this functionality, so there's a clear (or at least, clearer) separation of responsibilities.
  • Tests that use reflection to assert the existence/order/values of Checkstyle classes were explicitly called out with <allow> entries in import-control-test.xml. There may be room for additional improvement here in the future, but it is out of scope for this PR.
  • Tests that test Checkstyle APIs that have java.lang.reflect parameters or return values were explicitly allowed to import those classes with <allow> entries.

Usage of JUnit

  • assertThrows assertions were replaced with TestUtil. getExpectedThrowable, as per Wrap assertThrows with getExpectedThrowable method. #11343
  • All JUnit assertions other than assertDoesNotThrow were written with Truth assertions instead of JUnit assertions.
  • The import-control-test.xml syntax for disallowing JUnit and Hamcrest assertions was fixed.

Fixes #17910

@mureinik mureinik force-pushed the 17910-import-control-test-supression branch 2 times, most recently from cbe1333 to 25b6495 Compare October 11, 2025 13:19
@romani

romani commented Oct 11, 2025

Copy link
Copy Markdown
Member

@mureinik , thanks a lot for huge update. Impressed.

Please rebase to latest, as I just merged PR what used reflection to cover close-to-impossible situation by test.

Please squash all to single commit.

FYI:
https://github.com/checkstyle/checkstyle/wiki/How-to-run-certain-phases-and-validations

@mureinik mureinik force-pushed the 17910-import-control-test-supression branch from 25b6495 to 97028d7 Compare October 11, 2025 14:53
@mureinik

Copy link
Copy Markdown
Contributor Author

There are some CI failures around mutation coverage, looking in to it.

@mureinik mureinik force-pushed the 17910-import-control-test-supression branch 2 times, most recently from 83d9366 to 12ea33b Compare October 12, 2025 19:22
…h configuration in import-control-test.xml
@mureinik mureinik force-pushed the 17910-import-control-test-supression branch from 12ea33b to 21141a0 Compare October 12, 2025 20:02
@mureinik

mureinik commented Oct 12, 2025

Copy link
Copy Markdown
Contributor Author

@romani :

Please rebase to latest, as I just merged PR what used reflection to cover close-to-impossible situation by test.

done.

Please squash all to single commit.

and done.

with the latest revision of this PR, all CI checks pass.

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

Thanks a lot.
This code part did get much love from community for long time.
I appreciate your time , I know that passing our CI is not easy

@romani romani merged commit da46f7b into checkstyle:master Oct 13, 2025
120 checks passed
@romani

romani commented Oct 13, 2025

Copy link
Copy Markdown
Member

I appreciate your unexpected appearance in project please let me know how we how we can extend our collaboration.

@mureinik mureinik deleted the 17910-import-control-test-supression branch October 13, 2025 08:54
@mureinik

Copy link
Copy Markdown
Contributor Author

I appreciate your unexpected appearance in project please let me know how we how we can extend our collaboration.

Honestly, I just happened to have some downtime with the local public holidays, and wanted to see if I could help a project I've been using for years.

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.

Remove ImportControlTest supressions

2 participants