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.

Sign up for GitHub

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

Jump to bottom

Issue #14625: fix noinspectionreason for TestMethodWithoutAssertion #14812

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Issue #14625: fix noinspectionreason for TestMethodWithoutAssertion #14812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/test/java/com/puppycrawl/tools/checkstyle/internal/XdocsJavaDocsTest.java
View file
Open in desktop
Expand Up @@ -129,7 +129,8 @@ public void setUp() throws Exception {
* @noinspection JUnitTestMethodWithNoAssertions, TestMethodWithoutAssertion
* @noinspectionreason JUnitTestMethodWithNoAssertions - asserts in callstack,
* but not in this method
* @noinspectionreason TestMethodWithoutAssertion - until issue #14625
* @noinspectionreason TestMethodWithoutAssertion - asserts in callstack,
* but not in this method
Copy link
Member

Choose a reason for hiding this comment

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

We already have similar rule in some other tools, can you recheck if we suppressed this already for other tools maybe pmd or ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@romani , pmd yes....

checkstyle/config/pmd-test.xml

Lines 107 to 110 in bb56e9d

| //ClassOrInterfaceDeclaration[@SimpleName='XdocsPagesTest']
//MethodDeclaration[@Name='testAllChecksPresentOnAvailableChecksPage']
| //ClassOrInterfaceDeclaration[@SimpleName='XdocsJavaDocsTest']
//MethodDeclaration[@Name='testAllCheckSectionJavaDocs']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@romani , ping please.

Copy link
Member

Choose a reason for hiding this comment

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

Can it help if we rename examineCheckSection to assertCheckSection ? May be we can extend config to register this method name as asset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can it help if we rename examineCheckSection to assertCheckSection ? May be we can extend config to register this method name as asset.

theoretically, It should, but we would be extending config just for satisfying 2 methods

@romani , just pointing out, we have our methods named in chains.
examineCheckSection is followed by examineCheckSectionChildren and again examineCheckSubSection

checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/internal/XdocsJavaDocsTest.java

Line 165 in 1fc5987

private static void examineCheckSection(ModuleFactory moduleFactory, String fileName,

checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/internal/XdocsJavaDocsTest.java

Line 190 in 1fc5987

private static void examineCheckSectionChildren(Node section) {

checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/internal/XdocsJavaDocsTest.java

Line 206 in 1fc5987

private static void examineCheckSubSection(Node subSection, String subSectionName) {

Do we really want to extend our config to new method and hack all these just to workaround for one inspection that won't even impact users as much?

Please do correct me if I'm wrong, I don't mean to cross your suggestion 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@romani , ping please.

Copy link
Member

Choose a reason for hiding this comment

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

examineCheckSection is actually do assert by execution of
checker.process(files);

All Junit asserts are Custom Check that helps to parse java files.
We can add assert for checker to have no violations or errors after execution to testAllCheckSectionJavaDocs() as it is a field of class, so no suppression will be required.

*/
@Test
public void testAllCheckSectionJavaDocs() throws Exception {
Expand Down
5 changes: 3 additions & 2 deletions src/test/java/com/puppycrawl/tools/checkstyle/internal/XdocsPagesTest.java
View file
Open in desktop
Expand Up @@ -587,9 +587,10 @@ public void testAllCheckSections() throws Exception {
* Validates xml check documentation section.
*
* @noinspection JUnitTestMethodWithNoAssertions, TestMethodWithoutAssertion
* @noinspectionreason JUnitTestMethodWithNoAssertions -asserts in callstack,
* @noinspectionreason JUnitTestMethodWithNoAssertions - asserts in callstack,
* but not in this method
* @noinspectionreason TestMethodWithoutAssertion - asserts in callstack,
* but not in this method
* @noinspectionreason TestMethodWithoutAssertion - until issue #14625
*/
@Test
public void testAllCheckSectionsEx() throws Exception {
Expand Down