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.

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

Issue #14625: fix noinspectionreason for TestMethodWithoutAssertion #14812

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

Conversation

MANISH-K-07
Copy link
Contributor

@MANISH-K-07 MANISH-K-07 commented Apr 19, 2024

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>

Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

Question

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

| //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

private static void examineCheckSection(ModuleFactory moduleFactory, String fileName,

private static void examineCheckSectionChildren(Node section) {

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.

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

@romani romani left a comment

Choose a reason for hiding this comment

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

Ques

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

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
Copy link
Contributor Author

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

@MANISH-K-07 MANISH-K-07 requested a review from romani April 22, 2024 17:08
@MANISH-K-07
Copy link
Contributor Author

MANISH-K-07 commented Apr 30, 2024

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

@romani
Copy link
Member

romani commented May 11, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IDEA inspection suppressions resulted from migration to v2022.3.3
2 participants