SONARPHP-1569 S1448 should not raise on test classes (#1339)

rudy-regazzoni-sonarsource · web-flow · commit a75353b5a347 · 2024-12-09T16:47:27.000+01:00
diff --git a/its/ruling/src/integrationTest/resources/expected/PHPMailer/php-S1448.json b/its/ruling/src/integrationTest/resources/expected/PHPMailer/php-S1448.json
@@ -4,8 +4,5 @@
 ],
 "PHPMailer:src/SMTP.php": [
 31
-],
-"PHPMailer:test/PHPMailer/PHPMailerTest.php": [
-24
 ]
 }
diff --git a/its/ruling/src/integrationTest/resources/expected/PHPWord/php-S1448.json b/its/ruling/src/integrationTest/resources/expected/PHPWord/php-S1448.json
@@ -55,26 +55,5 @@
 ],
 "PHPWord:src/PhpWord/TemplateProcessor.php": [
 33
-],
-"PHPWord:tests/PhpWordTests/Metadata/SettingsTest.php": [
-31
-],
-"PHPWord:tests/PhpWordTests/Shared/HtmlTest.php": [
-36
-],
-"PHPWord:tests/PhpWordTests/Style/TextBoxTest.php": [
-30
-],
-"PHPWord:tests/PhpWordTests/TemplateProcessorTest.php": [
-39
-],
-"PHPWord:tests/PhpWordTests/Writer/Word2007/ElementTest.php": [
-31
-],
-"PHPWord:tests/PhpWordTests/Writer/Word2007/Part/DocumentTest.php": [
-33
-],
-"PHPWord:tests/PhpWordTests/Writer/Word2007/Part/SettingsTest.php": [
-33
 ]
 }
diff --git a/its/ruling/src/integrationTest/resources/expected/PHP_CodeSniffer/php-S1448.json b/its/ruling/src/integrationTest/resources/expected/PHP_CodeSniffer/php-S1448.json
@@ -8,23 +8,5 @@
 "PHP_CodeSniffer:src/Standards/PEAR/Tests/NamingConventions/ValidFunctionNameUnitTest.inc": [
 3,
 63
-],
-"PHP_CodeSniffer:tests/Core/ErrorSuppressionTest.php": [
-17
-],
-"PHP_CodeSniffer:tests/Core/File/FindEndOfStatementTest.php": [
-14
-],
-"PHP_CodeSniffer:tests/Core/File/FindStartOfStatementTest.php": [
-14
-],
-"PHP_CodeSniffer:tests/Core/File/GetMethodParametersTest.php": [
-14
-],
-"PHP_CodeSniffer:tests/Core/File/GetMethodPropertiesTest.php": [
-14
-],
-"PHP_CodeSniffer:tests/Core/Tokenizer/BackfillFnTokenTest.php": [
-14
 ]
 }
diff --git a/its/ruling/src/integrationTest/resources/expected/PhpSpreadsheet/php-S1448.json b/its/ruling/src/integrationTest/resources/expected/PhpSpreadsheet/php-S1448.json
@@ -151,41 +151,5 @@
 ],
 "PhpSpreadsheet:src/PhpSpreadsheet/Writer/Xlsx/Worksheet.php": [
 23
-],
-"PhpSpreadsheet:tests/PhpSpreadsheetTests/Calculation/CalculationTest.php": [
-15
-],
-"PhpSpreadsheet:tests/PhpSpreadsheetTests/Cell/CoordinateTest.php": [
-12
-],
-"PhpSpreadsheet:tests/PhpSpreadsheetTests/IOFactoryTest.php": [
-14
-],
-"PhpSpreadsheet:tests/PhpSpreadsheetTests/ReferenceHelperTest.php": [
-18
-],
-"PhpSpreadsheet:tests/PhpSpreadsheetTests/Shared/DateTest.php": [
-14
-],
-"PhpSpreadsheet:tests/PhpSpreadsheetTests/SpreadsheetTest.php": [
-12
-],
-"PhpSpreadsheet:tests/PhpSpreadsheetTests/Style/ConditionalFormatting/CellMatcherTest.php": [
-15
-],
-"PhpSpreadsheet:tests/PhpSpreadsheetTests/Worksheet/AutoFilter/AutoFilterTest.php": [
-12
-],
-"PhpSpreadsheet:tests/PhpSpreadsheetTests/Worksheet/Table/TableTest.php": [
-16
-],
-"PhpSpreadsheet:tests/PhpSpreadsheetTests/Worksheet/Worksheet2Test.php": [
-12
-],
-"PhpSpreadsheet:tests/PhpSpreadsheetTests/Worksheet/WorksheetTest.php": [
-18
-],
-"PhpSpreadsheet:tests/PhpSpreadsheetTests/Writer/Xlsx/ConditionalTest.php": [
-15
 ]
 }
diff --git a/its/ruling/src/integrationTest/resources/expected/RubixML/php-S1448.json b/its/ruling/src/integrationTest/resources/expected/RubixML/php-S1448.json
@@ -7,11 +7,5 @@
 ],
 "RubixML:src/Datasets/Unlabeled.php": [
 31
-],
-"RubixML:tests/Datasets/LabeledTest.php": [
-22
-],
-"RubixML:tests/Datasets/UnlabeledTest.php": [
-21
 ]
 }
diff --git a/its/ruling/src/integrationTest/resources/expected/flysystem/php-S1448.json b/its/ruling/src/integrationTest/resources/expected/flysystem/php-S1448.json
@@ -1,19 +1,10 @@
 {
-"flysystem:src/AdapterTestUtilities/FilesystemAdapterTestCase.php": [
-35
-],
 "flysystem:src/AsyncAwsS3/AsyncAwsS3Adapter.php": [
 46
 ],
-"flysystem:src/AsyncAwsS3/AsyncAwsS3AdapterTest.php": [
-34
-],
 "flysystem:src/AwsS3V3/AwsS3V3Adapter.php": [
 42
 ],
-"flysystem:src/AwsS3V3/AwsS3V3AdapterTest.php": [
-33
-],
 "flysystem:src/AwsS3V3/S3ClientStub.php": [
 22
 ],
@@ -23,36 +14,21 @@
 "flysystem:src/Filesystem.php": [
 18
 ],
-"flysystem:src/FilesystemTest.php": [
-25
-],
 "flysystem:src/Ftp/FtpAdapter.php": [
 37
 ],
-"flysystem:src/Ftp/FtpAdapterTestCase.php": [
-27
-],
 "flysystem:src/GoogleCloudStorage/GoogleCloudStorageAdapter.php": [
 46
 ],
 "flysystem:src/GridFS/GridFSAdapter.php": [
 33
 ],
-"flysystem:src/InMemory/InMemoryFilesystemAdapterTest.php": [
-22
-],
 "flysystem:src/Local/LocalFilesystemAdapter.php": [
 50
 ],
-"flysystem:src/Local/LocalFilesystemAdapterTest.php": [
-43
-],
 "flysystem:src/MountManager.php": [
 14
 ],
-"flysystem:src/MountManagerTest.php": [
-17
-],
 "flysystem:src/PathPrefixing/PathPrefixedAdapter.php": [
 30
 ],
@@ -67,8 +43,5 @@
 ],
 "flysystem:src/ZipArchive/ZipArchiveAdapter.php": [
 34
-],
-"flysystem:src/ZipArchive/ZipArchiveAdapterTestCase.php": [
-25
 ]
 }
diff --git a/its/ruling/src/integrationTest/resources/expected/psysh/php-S1448.json b/its/ruling/src/integrationTest/resources/expected/psysh/php-S1448.json
@@ -28,11 +28,5 @@
 ],
 "psysh:src/Shell.php": [
 51
-],
-"psysh:test/ConfigurationTest.php": [
-25
-],
-"psysh:test/ShellTest.php": [
-23
 ]
 }
diff --git a/php-checks/src/main/java/org/sonar/php/checks/OverridingMethodSimplyCallParentCheck.java b/php-checks/src/main/java/org/sonar/php/checks/OverridingMethodSimplyCallParentCheck.java
@@ -126,8 +126,7 @@ private void checkExpression(@Nullable ExpressionTree expressionTree, MethodDecl
   }
 
   private static boolean hasSameParameterList(MethodDeclarationTree method, MethodSymbol other) {
-    MethodSymbol methodSymbol = ((HasMethodSymbol) method).symbol();
-
+    var methodSymbol = ((HasMethodSymbol) method).symbol();
     List<Parameter> parameters = methodSymbol.parameters();
     List<Parameter> otherParameters = other.parameters();
     if (parameters.size() != otherParameters.size()) {
@@ -142,7 +141,7 @@ private static boolean hasSameParameterList(MethodDeclarationTree method, Method
   }
 
   private static boolean hasSameVisibilityAs(MethodDeclarationTree method, MethodSymbol other) {
-    return ((HasMethodSymbol) method).symbol().visibility().equals(other.visibility());
+    return ((HasMethodSymbol) method).symbol().visibility() == other.visibility();
   }
 
   private static boolean isFunctionCalledWithSameArgumentsAsDeclared(FunctionCallTree functionCallTree, MethodDeclarationTree method) {
diff --git a/php-checks/src/main/java/org/sonar/php/checks/TooManyMethodsInClassCheck.java b/php-checks/src/main/java/org/sonar/php/checks/TooManyMethodsInClassCheck.java
@@ -19,6 +19,7 @@
 import org.sonar.check.Rule;
 import org.sonar.check.RuleProperty;
 import org.sonar.php.api.PHPKeyword;
+import org.sonar.php.tree.symbols.HasMethodSymbol;
 import org.sonar.plugins.php.api.tree.Tree.Kind;
 import org.sonar.plugins.php.api.tree.declaration.ClassDeclarationTree;
 import org.sonar.plugins.php.api.tree.declaration.ClassMemberTree;
@@ -90,11 +91,14 @@ private int getNumberOfMethods(ClassTree tree) {
   }
 
   /**
-   * Return true if method is private or protected.
+   * Return true if method is private, protected or is a test method.
    */
   private boolean isExcluded(MethodDeclarationTree tree) {
-    if (!countNonpublicMethods) {
+    if (((HasMethodSymbol) tree).symbol().isTestMethod().isTrue()) {
+      return true;
+    }
 
+    if (!countNonpublicMethods) {
       for (SyntaxToken modifierToken : tree.modifiers()) {
         String modifier = modifierToken.text();
         if (PHPKeyword.PROTECTED.getValue().equals(modifier) || PHPKeyword.PRIVATE.getValue().equals(modifier)) {
diff --git a/php-checks/src/test/java/org/sonar/php/checks/TooManyMethodsInClassCheckTest.java b/php-checks/src/test/java/org/sonar/php/checks/TooManyMethodsInClassCheckTest.java
@@ -31,24 +31,24 @@ class TooManyMethodsInClassCheckTest {
   private static final String FILE_NAME = "TooManyMethodsInClassCheck.php";
 
   @Test
-  void defaultValue() throws Exception {
+  void shouldReportNoIssueOnDefaultValue() {
     CheckVerifier.verifyNoIssueIgnoringExpected(check, FILE_NAME);
   }
 
   @Test
-  void customMaximumMethodThreshold() throws Exception {
+  void shouldReportIssuesWithCustomThreshold() {
     check.maximumMethodThreshold = 2;
     CheckVerifier.verify(check, FILE_NAME);
   }
 
   @Test
-  void customCountNonPublicMethod() throws Exception {
+  void shouldReportIssuesWithCustomThresholdAndWithoutNonPublicMethods() {
     check.maximumMethodThreshold = 2;
     check.countNonpublicMethods = false;
 
     List<PhpIssue> issues = Arrays.asList(
       new LineIssue(check, 3, "Class \"I\" has 3 methods, which is greater than 2 authorized. Split it into smaller classes."),
-      new LineIssue(check, 35, "This anonymous class has 3 methods, which is greater than 2 authorized. Split it into smaller classes."));
+      new LineIssue(check, 44, "This anonymous class has 3 methods, which is greater than 2 authorized. Split it into smaller classes."));
     PHPCheckTest.check(check, TestUtils.getCheckFile(FILE_NAME), issues);
   }
 
diff --git a/php-checks/src/test/resources/checks/TooManyMethodsInClassCheck.php b/php-checks/src/test/resources/checks/TooManyMethodsInClassCheck.php
@@ -32,6 +32,15 @@ public function f1() {
   public function f2();
 }
 
+class TestClass {       // OK
+
+  public function test_f1();
+
+  public function f2();
+
+  public function f3();
+}
+
 $x = new class {       // Noncompliant
 //       ^^^^^
 
diff --git a/php-frontend/src/main/java/org/sonar/php/tree/impl/declaration/MethodDeclarationTreeImpl.java b/php-frontend/src/main/java/org/sonar/php/tree/impl/declaration/MethodDeclarationTreeImpl.java
@@ -130,6 +130,7 @@ public void setSymbol(MethodSymbol symbol) {
     this.symbol = symbol;
   }
 
+  @Override
   public MethodSymbol symbol() {
     return symbol;
   }

Original file line number	Diff line number	Diff line change
`@@ -4,8 +4,5 @@`
`4`	`4`	`],`
`5`	`5`	`"PHPMailer:src/SMTP.php": [`
`6`	`6`	`31`
`7`		`-],`
`8`		`-"PHPMailer:test/PHPMailer/PHPMailerTest.php": [`
`9`		`-24`
`10`	`7`	`]`
`11`	`8`	`}`
Original file line number	Diff line number	Diff line change
`@@ -7,11 +7,5 @@`
`7`	`7`	`],`
`8`	`8`	`"RubixML:src/Datasets/Unlabeled.php": [`
`9`	`9`	`31`
`10`		`-],`
`11`		`-"RubixML:tests/Datasets/LabeledTest.php": [`
`12`		`-22`
`13`		`-],`
`14`		`-"RubixML:tests/Datasets/UnlabeledTest.php": [`
`15`		`-21`
`16`	`10`	`]`
`17`	`11`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1,19 +1,10 @@`
`1`	`1`	`{`
`2`		`-"flysystem:src/AdapterTestUtilities/FilesystemAdapterTestCase.php": [`
`3`		`-35`
`4`		`-],`
`5`	`2`	`"flysystem:src/AsyncAwsS3/AsyncAwsS3Adapter.php": [`
`6`	`3`	`46`
`7`	`4`	`],`
`8`		`-"flysystem:src/AsyncAwsS3/AsyncAwsS3AdapterTest.php": [`
`9`		`-34`
`10`		`-],`
`11`	`5`	`"flysystem:src/AwsS3V3/AwsS3V3Adapter.php": [`
`12`	`6`	`42`
`13`	`7`	`],`
`14`		`-"flysystem:src/AwsS3V3/AwsS3V3AdapterTest.php": [`
`15`		`-33`
`16`		`-],`
`17`	`8`	`"flysystem:src/AwsS3V3/S3ClientStub.php": [`
`18`	`9`	`22`
`19`	`10`	`],`
`@@ -23,36 +14,21 @@`
`23`	`14`	`"flysystem:src/Filesystem.php": [`
`24`	`15`	`18`
`25`	`16`	`],`
`26`		`-"flysystem:src/FilesystemTest.php": [`
`27`		`-25`
`28`		`-],`
`29`	`17`	`"flysystem:src/Ftp/FtpAdapter.php": [`
`30`	`18`	`37`
`31`	`19`	`],`
`32`		`-"flysystem:src/Ftp/FtpAdapterTestCase.php": [`
`33`		`-27`
`34`		`-],`
`35`	`20`	`"flysystem:src/GoogleCloudStorage/GoogleCloudStorageAdapter.php": [`
`36`	`21`	`46`
`37`	`22`	`],`
`38`	`23`	`"flysystem:src/GridFS/GridFSAdapter.php": [`
`39`	`24`	`33`
`40`	`25`	`],`
`41`		`-"flysystem:src/InMemory/InMemoryFilesystemAdapterTest.php": [`
`42`		`-22`
`43`		`-],`
`44`	`26`	`"flysystem:src/Local/LocalFilesystemAdapter.php": [`
`45`	`27`	`50`
`46`	`28`	`],`
`47`		`-"flysystem:src/Local/LocalFilesystemAdapterTest.php": [`
`48`		`-43`
`49`		`-],`
`50`	`29`	`"flysystem:src/MountManager.php": [`
`51`	`30`	`14`
`52`	`31`	`],`
`53`		`-"flysystem:src/MountManagerTest.php": [`
`54`		`-17`
`55`		`-],`
`56`	`32`	`"flysystem:src/PathPrefixing/PathPrefixedAdapter.php": [`
`57`	`33`	`30`
`58`	`34`	`],`
`@@ -67,8 +43,5 @@`
`67`	`43`	`],`
`68`	`44`	`"flysystem:src/ZipArchive/ZipArchiveAdapter.php": [`
`69`	`45`	`34`
`70`		`-],`
`71`		`-"flysystem:src/ZipArchive/ZipArchiveAdapterTestCase.php": [`
`72`		`-25`
`73`	`46`	`]`
`74`	`47`	`}`
Original file line number	Diff line number	Diff line change
`@@ -28,11 +28,5 @@`
`28`	`28`	`],`
`29`	`29`	`"psysh:src/Shell.php": [`
`30`	`30`	`51`
`31`		`-],`
`32`		`-"psysh:test/ConfigurationTest.php": [`
`33`		`-25`
`34`		`-],`
`35`		`-"psysh:test/ShellTest.php": [`
`36`		`-23`
`37`	`31`	`]`
`38`	`32`	`}`
Original file line number	Diff line number	Diff line change
`@@ -126,8 +126,7 @@ private void checkExpression(@Nullable ExpressionTree expressionTree, MethodDecl`
`126`	`126`	`}`
`127`	`127`
`128`	`128`	`private static boolean hasSameParameterList(MethodDeclarationTree method, MethodSymbol other) {`
`129`		`- MethodSymbol methodSymbol = ((HasMethodSymbol) method).symbol();`
`130`		`-`
	`129`	`+ var methodSymbol = ((HasMethodSymbol) method).symbol();`
`131`	`130`	`List<Parameter> parameters = methodSymbol.parameters();`
`132`	`131`	`List<Parameter> otherParameters = other.parameters();`
`133`	`132`	`if (parameters.size() != otherParameters.size()) {`
`@@ -142,7 +141,7 @@ private static boolean hasSameParameterList(MethodDeclarationTree method, Method`
`142`	`141`	`}`
`143`	`142`
`144`	`143`	`private static boolean hasSameVisibilityAs(MethodDeclarationTree method, MethodSymbol other) {`
`145`		`- return ((HasMethodSymbol) method).symbol().visibility().equals(other.visibility());`
	`144`	`+ return ((HasMethodSymbol) method).symbol().visibility() == other.visibility();`
`146`	`145`	`}`
`147`	`146`
`148`	`147`	`private static boolean isFunctionCalledWithSameArgumentsAsDeclared(FunctionCallTree functionCallTree, MethodDeclarationTree method) {`