Skip to content

Commit 467df42

Browse files
committed
Java: Renamed query java/mocking-all-non-private-methods-means-unit-test-is-too-big to java/excessive-public-method-mocking and changed wording from non-private to public
1 parent ff648fc commit 467df42

File tree

15 files changed

+242
-8
lines changed

15 files changed

+242
-8
lines changed

java/ql/integration-tests/java/query-suite/java-code-quality-extended.qls.expected

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ ql/java/ql/src/Likely Bugs/Concurrency/ScheduledThreadPoolExecutorZeroThread.ql
3737
ql/java/ql/src/Likely Bugs/Concurrency/SynchOnBoxedType.ql
3838
ql/java/ql/src/Likely Bugs/Concurrency/SynchSetUnsynchGet.ql
3939
ql/java/ql/src/Likely Bugs/Frameworks/JUnit/JUnit5MissingNestedAnnotation.ql
40-
ql/java/ql/src/Likely Bugs/Frameworks/JUnit/MockingAllNonPrivateMethodsMeansUnitTestIsTooBig.ql
40+
ql/java/ql/src/Likely Bugs/Frameworks/JUnit/ExcessivePublicMethodMocking.ql
4141
ql/java/ql/src/Likely Bugs/Inheritance/NoNonFinalInConstructor.ql
4242
ql/java/ql/src/Likely Bugs/Likely Typos/ContainerSizeCmpZero.ql
4343
ql/java/ql/src/Likely Bugs/Likely Typos/ContradictoryTypeChecks.ql

java/ql/integration-tests/java/query-suite/java-code-quality.qls.expected

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ ql/java/ql/src/Likely Bugs/Concurrency/ScheduledThreadPoolExecutorZeroThread.ql
3535
ql/java/ql/src/Likely Bugs/Concurrency/SynchOnBoxedType.ql
3636
ql/java/ql/src/Likely Bugs/Concurrency/SynchSetUnsynchGet.ql
3737
ql/java/ql/src/Likely Bugs/Frameworks/JUnit/JUnit5MissingNestedAnnotation.ql
38-
ql/java/ql/src/Likely Bugs/Frameworks/JUnit/MockingAllNonPrivateMethodsMeansUnitTestIsTooBig.ql
38+
ql/java/ql/src/Likely Bugs/Frameworks/JUnit/ExcessivePublicMethodMocking.ql
3939
ql/java/ql/src/Likely Bugs/Inheritance/NoNonFinalInConstructor.ql
4040
ql/java/ql/src/Likely Bugs/Likely Typos/ContainerSizeCmpZero.ql
4141
ql/java/ql/src/Likely Bugs/Likely Typos/ContradictoryTypeChecks.ql
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
## Overview
22

3-
Mocking methods of a class is necessary for unit tests to run without overhead caused by expensive I/O operations. However, when a unit test ends up mocking all non-private methods of a class, it may indicate that the test is too complicated, possibly because it is trying to test multiple things at once. Such extensive mocking is likely a signal that the scope of the unit test is reaching beyond a single unit of functionality.
3+
Mocking methods of a class is necessary for unit tests to run without overhead caused by expensive I/O operations. However, when a unit test ends up mocking all public methods of a class, it may indicate that the test is too complicated, possibly because it is trying to test multiple things at once. Such extensive mocking is likely a signal that the scope of the unit test is reaching beyond a single unit of functionality.
44

55
## Recommendation
66

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
/**
2-
* @id java/mocking-all-non-private-methods-means-unit-test-is-too-big
3-
* @name Mocking all non-private methods of a class may indicate the unit test is testing too much
4-
* @description Mocking all non-private methods provided by a class might indicate the unit test
2+
* @id java/excessive-public-method-mocking
3+
* @previous-id java/mocking-all-non-private-methods-means-unit-test-is-too-big
4+
* @name Mocking all public methods of a class may indicate the unit test is testing too much
5+
* @description Mocking all public methods provided by a class might indicate the unit test
56
* aims to test too many things.
67
* @kind problem
78
* @precision high
Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
/**
2+
* @id java/visible-for-testing-abuse
3+
* @name Use of VisibleForTesting in production code
4+
* @description Accessing methods, fields or classes annotated with `@VisibleForTesting` from
5+
* production code goes against the intention of the annotation and may indicate
6+
* programmer error.
7+
* @kind problem
8+
* @precision high
9+
* @problem.severity warning
10+
* @tags quality
11+
* maintainability
12+
* readability
13+
*/
14+
15+
import java
16+
17+
/**
18+
* Holds if a `Callable` is within the same type hierarchy as `RefType`
19+
* (including through lambdas, inner classes, and outer classes)
20+
*/
21+
predicate isWithinType(Callable c, RefType t) {
22+
// Either the callable is in the target type, or they share a common enclosing type
23+
c.getDeclaringType().getEnclosingType*() = t.getEnclosingType*()
24+
}
25+
26+
/**
27+
* Holds if `e` is within the same package as `t`
28+
*/
29+
predicate isWithinPackage(Expr e, RefType t) {
30+
e.getCompilationUnit().getPackage() = t.getPackage()
31+
}
32+
33+
/**
34+
* Holds if a callable or any of its enclosing callables is annotated with @VisibleForTesting
35+
*/
36+
predicate isWithinVisibleForTestingContext(Callable c) {
37+
c.getAnAnnotation().getType().hasName("VisibleForTesting")
38+
or
39+
isWithinVisibleForTestingContext(c.getEnclosingCallable())
40+
}
41+
42+
from Annotatable annotated, Expr e
43+
where
44+
annotated.getAnAnnotation().getType().hasName("VisibleForTesting") and
45+
(
46+
// field access
47+
e =
48+
any(FieldAccess v |
49+
v.getField() = annotated and
50+
// depending on the visibility of the field, using the annotation to abuse the visibility may/may not be occurring
51+
(
52+
// if its package protected report when its used outside its class because it should have been private (class only permitted)
53+
v.getField().isPackageProtected() and
54+
not isWithinType(v.getEnclosingCallable(), v.getField().getDeclaringType())
55+
or
56+
// if public or protected report when its used outside its package because package protected should have been enough (package only permitted)
57+
(v.getField().isPublic() or v.getField().isProtected()) and
58+
not isWithinPackage(v, v.getField().getDeclaringType())
59+
)
60+
)
61+
or
62+
// method access
63+
e =
64+
any(MethodCall c |
65+
c.getMethod() = annotated and
66+
// depending on the visibility of the method, using the annotation to abuse the visibility may/may not be occurring
67+
(
68+
// if its package protected report when its used outside its class because it should have been private (class only permitted)
69+
c.getMethod().isPackageProtected() and
70+
not isWithinType(c.getEnclosingCallable(), c.getMethod().getDeclaringType())
71+
or
72+
// if public or protected report when its used outside its package because package protected should have been enough (package only permitted)
73+
(c.getMethod().isPublic() or c.getMethod().isProtected()) and
74+
not isWithinPackage(c, c.getMethod().getDeclaringType())
75+
)
76+
)
77+
or
78+
// Class instantiation - report if used outside appropriate scope
79+
e =
80+
any(ClassInstanceExpr c |
81+
c.getConstructedType() = annotated and
82+
(
83+
c.getConstructedType().isPublic() and not isWithinPackage(c, c.getConstructedType())
84+
or
85+
c.getConstructedType().hasNoModifier() and
86+
c.getConstructedType() instanceof NestedClass and
87+
not isWithinType(c.getEnclosingCallable(), c.getConstructedType())
88+
)
89+
)
90+
) and
91+
// not in a test where use is appropriate
92+
not e.getEnclosingCallable() instanceof LikelyTestMethod and
93+
// not when the accessing method or any enclosing method is @VisibleForTesting (test-to-test communication)
94+
not isWithinVisibleForTestingContext(e.getEnclosingCallable()) and
95+
// not when used in annotation contexts
96+
not e.getParent*() instanceof Annotation
97+
select e, "Access of $@ annotated with VisibleForTesting found in production code.", annotated,
98+
"element"
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
query: Likely Bugs/Frameworks/JUnit/ExcessivePublicMethodMocking.ql
2+
postprocess: utils/test/InlineExpectationsTestQuery.ql

0 commit comments

Comments
 (0)