Skip to content

Commit 1dfdb7b

Browse files
yegor256claude
andauthored
#1606 Match only known assert methods in UnitTestContainsTooManyAssertsRule (#1610)
The rule previously counted any invocation whose simple name starts with 'assert', 'check' or 'verify' (via PMD's TestFrameworksUtil) as an assertion, producing false positives for unrelated APIs such as pull.checks() (jcabi-github) and Mockito.verify(...). Replace the prefix match with an explicit allowlist of JUnit and Hamcrest assertion method names so that only real assertions are counted. Co-authored-by: Claude <noreply@anthropic.com>
1 parent 5a03c3a commit 1dfdb7b

4 files changed

Lines changed: 109 additions & 14 deletions

File tree

src/main/java/com/qulice/pmd/rules/UnitTestContainsTooManyAssertsRule.java

Lines changed: 37 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
*/
55
package com.qulice.pmd.rules;
66

7+
import java.util.Set;
78
import net.sourceforge.pmd.lang.java.ast.ASTBlock;
89
import net.sourceforge.pmd.lang.java.ast.ASTMethodCall;
910
import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
@@ -13,16 +14,46 @@
1314
/**
1415
* Rule to check that JUnit/TestNG test methods do not contain more than
1516
* one assertion. Unlike the PMD built-in
16-
* {@code UnitTestContainsTooManyAsserts} rule, this implementation does
17-
* not count calls to {@code assertThrows} as assertions, because a
18-
* common idiom wraps {@code assertThrows(...).getMessage()} inside an
17+
* {@code UnitTestContainsTooManyAsserts} rule, this implementation only
18+
* counts calls whose simple name is one of the known JUnit/Hamcrest
19+
* assertion methods, instead of counting any identifier with an
20+
* {@code assert}, {@code check} or {@code verify} prefix. The PMD
21+
* default produces false positives for unrelated APIs such as
22+
* {@code pull.checks()} (jcabi-github) or {@code Mockito.verify(...)},
23+
* and the {@code assertThrows} method is also excluded because a common
24+
* idiom wraps {@code assertThrows(...).getMessage()} inside an
1925
* {@code assertThat} to verify the thrown exception's message in a
2026
* single logical check.
2127
* @since 0.26.0
2228
*/
2329
public final class UnitTestContainsTooManyAssertsRule
2430
extends AbstractJavaRulechainRule {
2531

32+
/**
33+
* Method names recognised as JUnit / Hamcrest assertions. The list
34+
* intentionally excludes {@code assertThrows}, which is treated as
35+
* an exception-capturing helper, not an assertion.
36+
*/
37+
private static final Set<String> ASSERTIONS = Set.of(
38+
"assertThat",
39+
"assertEquals",
40+
"assertNotEquals",
41+
"assertTrue",
42+
"assertFalse",
43+
"assertNull",
44+
"assertNotNull",
45+
"assertSame",
46+
"assertNotSame",
47+
"assertArrayEquals",
48+
"assertIterableEquals",
49+
"assertLinesMatch",
50+
"assertDoesNotThrow",
51+
"assertTimeout",
52+
"assertTimeoutPreemptively",
53+
"assertAll",
54+
"fail"
55+
);
56+
2657
public UnitTestContainsTooManyAssertsRule() {
2758
super(ASTMethodDeclaration.class);
2859
}
@@ -41,7 +72,8 @@ public Object visit(final ASTMethodDeclaration method, final Object data) {
4172
}
4273

4374
private static boolean isCountedAssert(final ASTMethodCall call) {
44-
return TestFrameworksUtil.isProbableAssertCall(call)
45-
&& !"assertThrows".equals(call.getMethodName());
75+
return UnitTestContainsTooManyAssertsRule.ASSERTIONS.contains(
76+
call.getMethodName()
77+
);
4678
}
4779
}

src/main/resources/com/qulice/pmd/ruleset.xml

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -38,13 +38,16 @@
3838
-->
3939
<exclude name="UnitTestShouldIncludeAssert"/>
4040
<!--
41-
UnitTestContainsTooManyAsserts is excluded because it counts
42-
Assertions.assertThrows as an assertion, producing false positives
43-
when a test wraps assertThrows(...).getMessage() inside assertThat
44-
to verify the thrown exception's message in a single logical check.
45-
Until PMD supports excluding method names, we use our own
46-
UnitTestContainsTooManyAssertsRule that ignores assertThrows.
41+
UnitTestContainsTooManyAsserts is excluded because it counts any
42+
method invocation whose simple name starts with 'assert', 'check'
43+
or 'verify', producing false positives for unrelated APIs such as
44+
pull.checks() (jcabi-github) or Mockito.verify(...). It also
45+
counts Assertions.assertThrows, which breaks the common idiom of
46+
wrapping assertThrows(...).getMessage() inside assertThat. We use
47+
our own UnitTestContainsTooManyAssertsRule that only counts the
48+
known JUnit/Hamcrest assertion methods and ignores assertThrows.
4749
Downstream: https://github.com/yegor256/qulice/issues/1519
50+
Downstream: https://github.com/yegor256/qulice/issues/1606
4851
-->
4952
<exclude name="UnitTestContainsTooManyAsserts"/>
5053
<exclude name="UnusedPrivateField"/>
@@ -271,9 +274,14 @@
271274
</rule>
272275
<rule name="UnitTestContainsTooManyAsserts" message="Unit tests should not contain more than 1 assert(s)." language="java" class="com.qulice.pmd.rules.UnitTestContainsTooManyAssertsRule">
273276
<description>
274-
Unit tests should not contain more than one assertion. This
275-
variant of PMD's UnitTestContainsTooManyAsserts rule does not
276-
count calls to assertThrows as assertions, so a test that wraps
277+
Unit tests should not contain more than one assertion. Unlike
278+
PMD's UnitTestContainsTooManyAsserts rule, this variant only
279+
counts calls whose simple name is one of the known JUnit /
280+
Hamcrest assertion methods (assertThat, assertEquals, assertTrue,
281+
fail, ...) instead of counting any identifier with an 'assert',
282+
'check' or 'verify' prefix. Calls such as Mockito.verify(...) or
283+
pull.checks() are therefore not flagged. Calls to assertThrows
284+
are also not counted, so a test that wraps
277285
assertThrows(...).getMessage() inside an assertThat to verify the
278286
thrown exception's message is not flagged.
279287
</description>

src/test/java/com/qulice/pmd/PmdAssertionsTest.java

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,26 @@ void reportsTooManyAssertsEvenWithAssertThrows() throws Exception {
112112
).assertOk();
113113
}
114114

115+
/**
116+
* PmdValidator does not report UnitTestContainsTooManyAsserts when a test
117+
* has a single assertion combined with calls such as
118+
* {@code Mockito.verify(...)} or {@code pull.checks()} whose simple name
119+
* starts with 'verify' or 'check' but which are not JUnit/Hamcrest
120+
* assertions.
121+
* Regression test for https://github.com/yegor256/qulice/issues/1606
122+
* @throws Exception If something wrong happens inside.
123+
*/
124+
@Test
125+
void allowsMockitoVerifyAndChecksCalls() throws Exception {
126+
new PmdAssert(
127+
"AssertThatWithMockitoVerifyAndChecks.java",
128+
Matchers.any(Boolean.class),
129+
Matchers.not(
130+
Matchers.containsString("UnitTestContainsTooManyAsserts")
131+
)
132+
).assertOk();
133+
}
134+
115135
/**
116136
* PmdValidator can allow only package private methods, marked with: Test,
117137
* RepeatedTest, TestFactory, TestTemplate or ParameterizedTest annotations.
Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
/*
2+
* SPDX-FileCopyrightText: Copyright (c) 2011-2026 Yegor Bugayenko
3+
* SPDX-License-Identifier: MIT
4+
*/
5+
package foo;
6+
7+
import org.hamcrest.MatcherAssert;
8+
import org.hamcrest.Matchers;
9+
import org.junit.jupiter.api.Test;
10+
import org.mockito.Mockito;
11+
12+
final class AssertThatWithMockitoVerifyAndChecksTest {
13+
14+
@Test
15+
void detectsSuccess() {
16+
final Pull pull = new Pull();
17+
final Observer observer = Mockito.mock(Observer.class);
18+
MatcherAssert.assertThat("ok", true, Matchers.is(true));
19+
final MkChecks checks = (MkChecks) pull.checks();
20+
Mockito.verify(observer).onSuccess();
21+
}
22+
23+
private static final class Pull {
24+
Object checks() {
25+
return null;
26+
}
27+
}
28+
29+
private static final class MkChecks {
30+
}
31+
32+
private interface Observer {
33+
void onSuccess();
34+
}
35+
}

0 commit comments

Comments
 (0)