Issue #14625: fix noinspectionreason for TestMethodWithoutAssertion by MANISH-K-07 · Pull Request #14812 · checkstyle/checkstyle

MANISH-K-07 · 2024-04-19T14:02:55Z

Resolves #14625

TestMethodWithoutAssertion inspection is a permanent suppression for XdocsJavaDocsTest & XdocsPagesTest.

Reason being same as for inspection JUnitTestMethodWithNoAssertions.

     * @noinspectionreason JUnitTestMethodWithNoAssertions - asserts in callstack,
     *      but not in this method

Ideally, in this case, both inspections function the same, I believe.

<problems is_local_tool="true">
<problem>
<file>file://$PROJECT_DIR$/src/test/java/com/puppycrawl/tools/checkstyle/internal/XdocsJavaDocsTest.java</file>
<line>134</line>
<module>project</module>
<package>com.puppycrawl.tools.checkstyle.internal</package>
<entry_point TYPE="method" FQNAME="com.puppycrawl.tools.checkstyle.internal.XdocsJavaDocsTest void testAllCheckSectionJavaDocs()"/>
<problem_class id="TestMethodWithoutAssertion" severity="ERROR" attribute_key="ERRORS_ATTRIBUTES">Test method without assertions</problem_class>
<description>Test method <code>testAllCheckSectionJavaDocs()</code> contains no assertions #loc</description>
<highlighted_element>testAllCheckSectionJavaDocs</highlighted_element>
<language>JAVA</language>
<offset>16</offset>
<length>27</length>
</problem>

<problem>
<file>file://$PROJECT_DIR$/src/test/java/com/puppycrawl/tools/checkstyle/internal/XdocsPagesTest.java</file>
<line>594</line>
<module>project</module>
<package>com.puppycrawl.tools.checkstyle.internal</package>
<entry_point TYPE="method" FQNAME="com.puppycrawl.tools.checkstyle.internal.XdocsPagesTest void testAllCheckSectionsEx()"/>
<problem_class id="TestMethodWithoutAssertion" severity="ERROR" attribute_key="ERRORS_ATTRIBUTES">Test method without assertions</problem_class>
<description>Test method <code>testAllCheckSectionsEx()</code> contains no assertions #loc</description>
<highlighted_element>testAllCheckSectionsEx</highlighted_element>
<language>JAVA</language>
<offset>16</offset>
<length>22</length>
</problem>
</problems>

…ssertion

romani

Question

romani · 2024-04-19T15:37:58Z

     *      but not in this method
-     * @noinspectionreason TestMethodWithoutAssertion - until issue #14625
+     * @noinspectionreason TestMethodWithoutAssertion - asserts in callstack,
+     *      but not in this method


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

@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']

@romani , ping please.

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

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 😃

@romani , ping please.

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.

romani

Ques

romani · 2024-04-22T15:45:18Z

     *      but not in this method
-     * @noinspectionreason TestMethodWithoutAssertion - until issue #14625
+     * @noinspectionreason TestMethodWithoutAssertion - asserts in callstack,
+     *      but not in this method


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

MANISH-K-07 · 2024-04-22T17:04:35Z

@romani , please see post above at #14812 (comment)

MANISH-K-07 · 2024-04-30T12:33:47Z

@romani , could you please assign this PR to yourself while we discuss how to proceed..

romani · 2024-05-11T14:28:20Z

@MANISH-K-07, ping, please find time to finish this PR.

romani · 2024-06-09T17:10:22Z

@MANISH-K-07, ping, please find time to finish this PR.

romani · 2024-09-17T15:41:53Z

How to configure inspection:
https://stackoverflow.com/q/48034643/1015848

https://www.jetbrains.com.cn/en-us/help/inspectopedia/TestMethodWithoutAssertion.html

We just need change name of method examineCheckSection only, suggestion assertCheckSection

romani · 2024-10-18T15:25:23Z

Everyone is welcome to continue this work.

romani · 2024-11-06T02:24:23Z

@MohanadKh03, while I am working on n your PR with write tag.
Can you help us to move this fix across finish line ?

MohanadKh03 · 2024-11-06T10:41:29Z

Sure
Just for clarifications on what is left to do , it seems the only change needed is to just change the method's name and we can test this by configuring jetbrains' inspection right ?

romani · 2024-11-06T12:06:18Z

Yes

MohanadKh03 · 2024-11-06T13:21:14Z

Actually we do not need to rename do we ? I tried with and without renaming and since it was suppressed , both work since this is suppressed anyway so unless we are removing the suppression and extending it to config then we could also extend the old name right ? Also by extending the config does this mean adding it to the whitelist or what kind of extension exactly ?

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

As it stands right now this can be merged if we are going to keep the suppression I dont really see any point of renaming

romani · 2024-11-06T13:37:43Z

we should remove suppression, this is whole reason of issue.
after removal, CI will start to fail, and complain on method, method need to be renamed, and possibly adjust inspection config if required.

romani · 2024-11-06T16:27:00Z

Moved to #15884

Issue checkstyle#14625: fix noinspectionreason for TestMethodWithoutA…

8ca9668

…ssertion

romani requested changes Apr 19, 2024

View reviewed changes

MANISH-K-07 requested a review from romani April 20, 2024 02:50

romani reviewed Apr 22, 2024

View reviewed changes

MANISH-K-07 requested a review from romani April 22, 2024 17:08

nrmancuso assigned romani May 7, 2024

romani added the awaiting changes label May 11, 2024

romani mentioned this pull request May 17, 2024

IDEA inspection suppressions resulted from migration to v2022.3.3 #14625

Closed

4 tasks

romani removed their assignment Jun 23, 2024

romani added the abandoned label Oct 18, 2024

MohanadKh03 mentioned this pull request Nov 6, 2024

Issue #14625: fix noinspectionreason for TestMethodWithoutAssertion #15884

Merged

romani closed this Nov 6, 2024

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

Uh oh!

Conversation

MANISH-K-07 commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

romani Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

MANISH-K-07 Apr 20, 2024

Choose a reason for hiding this comment

Uh oh!

MANISH-K-07 Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

romani Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

MANISH-K-07 Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MANISH-K-07 Apr 27, 2024

Choose a reason for hiding this comment

Uh oh!

romani May 7, 2024

Choose a reason for hiding this comment

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

romani Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

MANISH-K-07 commented Apr 22, 2024

Uh oh!

MANISH-K-07 commented Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani commented May 11, 2024

Uh oh!

romani commented Jun 9, 2024

Uh oh!

romani commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani commented Oct 18, 2024

Uh oh!

romani commented Nov 6, 2024

Uh oh!

MohanadKh03 commented Nov 6, 2024

Uh oh!

romani commented Nov 6, 2024

Uh oh!

MohanadKh03 commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani commented Nov 6, 2024

Uh oh!

romani commented Nov 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MANISH-K-07 commented Apr 19, 2024 •

edited

Loading

MANISH-K-07 Apr 22, 2024 •

edited

Loading

MANISH-K-07 commented Apr 30, 2024 •

edited

Loading

romani commented Sep 17, 2024 •

edited

Loading

MohanadKh03 commented Nov 6, 2024 •

edited

Loading