Qual: Refactor CodingPhpTest / CommonClassTest - less verbosity, but better when error (#28388)

* Qual: CommonClassTest - less verbosity, but better when error

# Qual: CommonClassTest - less verbosity, but better when error

- Report the test method and parameters in case of error.
- Less verbosity about setup.
- $_ENV was empty array, replaced with getenv()

* Qual: Refactor CodingPhpTest

# Qual: Refactor CodingPhpTest

- Use dataprovider (better progress report, better errors, better continuation)
- Use dol_dir_list's exclude_filter capability (do not traverse the excluded dirs)
- Reduce debug output from dolibarr.log (not really relevant for these tests).

* Fix: References to loop variables outside loop

# Fix: References to loop variables outside loop

The test referenced some undefined variables outside foreach loops ($val[0])

* Update CommonClassTest.class.php

---------

Co-authored-by: Laurent Destailleur <eldy@destailleur.fr>

test/phpunit/CodingPhpTest.php

@@ -81,42 +81,53 @@ $conf->global->MAIN_DISABLE_ALL_MAILS = 1;
  */
 class CodingPhpTest extends CommonClassTest
 {
+	/**
+	 * Return list of files for which to verify Php checks
+	 *
+	 * @return array{name:string,path:string,level1name:string,relativename:string,fullname:string,date:string,size:int,perm:int,type:string} List of php files to check (dol_dir_list)
+	 */
+	public function phpFilesProvider()
+	{
+		// File functions are needed
+		include_once DOL_DOCUMENT_ROOT.'/core/lib/files.lib.php';
+
+
+		$excludeRegexList
+			= array(
+				'\/includes\/',
+				'\/install\/doctemplates\/websites\/',
+				'\/custom\/',
+				'\/dolimed',
+				'\/nltechno',
+				'\/teclib',
+			);
+		$fullRegex = '(?:'.implode('|', $excludeRegexList).')';
+		$filesarray = dol_dir_list(DOL_DOCUMENT_ROOT, 'files', 1, '\.php', [$fullRegex], 'fullname', SORT_ASC, 0, 1, '', 1);
+
+		/*
+		$filteredArray =  array_filter(
+			$filesarray,
+			static function($file) use (&$fullRegex) {
+				return !preg_match($fullRegex, $file['relativename']);
+			}
+		));
+		*/
+		return array_map(function ($value) {
+			return array($value);
+		}, $filesarray);
+	}
+
 	/**
 	 * testPHP
 	 *
+	 * @param array{name:string,path:string,level1name:string,relativename:string,fullname:string,date:string,size:int,perm:int,type:string} $file File information
 	 * @return string
+	 *
+	 * @dataProvider phpFilesProvider
 	 */
-	public function testPHP()
+	public function testPHP($file)
 	{
-		global $conf,$user,$langs,$db;
+		$this->nbLinesToShow = 1;
-		$conf = $this->savconf;
-		$user = $this->savuser;
-		$langs = $this->savlangs;
-		$db = $this->savdb;
-
-		include_once DOL_DOCUMENT_ROOT.'/core/lib/files.lib.php';
-		$filesarray = dol_dir_list(DOL_DOCUMENT_ROOT, 'files', 1, '\.php', null, 'fullname', SORT_ASC, 0, 1, '', 1);
-
-		foreach ($filesarray as $key => $file) {
-			if (preg_match('/\/(htdocs|html)\/includes\//', $file['fullname'])) {
-				continue;
-			}
-			if (preg_match('/\/(htdocs|html)\/install\/doctemplates\/websites\//', $file['fullname'])) {
-				continue;
-			}
-			if (preg_match('/\/(htdocs|html)\/custom\//', $file['fullname'])) {
-				continue;
-			}
-			if (preg_match('/\/(htdocs|html)\/dolimed/', $file['fullname'])) {
-				continue;
-			}
-			if (preg_match('/\/(htdocs|html)\/nltechno/', $file['fullname'])) {
-				continue;
-			}
-			if (preg_match('/\/(htdocs|html)\/teclib/', $file['fullname'])) {
-				continue;
-			}
-
 		//print 'Check php file '.$file['relativename']."\n";
 		$filecontent = file_get_contents($file['fullname']);
 
@@ -368,24 +379,28 @@ class CodingPhpTest extends CommonClassTest
 		// Check string sql|set|WHERE|...'".$yyy->xxx   with xxx that is not 'escape', 'idate', .... It means we forget a db->escape when forging sql request.
 		$ok = true;
 		$matches = array();
+		$found = "";
 		preg_match_all('/(sql|SET|WHERE|INSERT|VALUES|LIKE).+\s*\'"\s*\.\s*\$(.......)/', $filecontent, $matches, PREG_SET_ORDER);
 		foreach ($matches as $key => $val) {
 			if (! in_array($val[2], array('this->d', 'this->e', 'db->esc', 'dbs->es', 'dbs->id', 'mydb->e', 'dbsessi', 'db->ida', 'escaped', 'exclude', 'include'))) {
+				$found = $val[0];
 				$ok = false;	// This will generate error
 				break;
 			}
 			//if ($reg[0] != 'db') $ok=false;
 		}
 		//print __METHOD__." Result for checking we don't have non escaped string in sql requests for file ".$file."\n";
-			$this->assertTrue($ok, 'Found non escaped string in building of a sql request (case 2) in '.$file['relativename'].': '.$val[0].' - Bad.');
+		$this->assertTrue($ok, 'Found non escaped string in building of a sql request (case 2) in '.$file['relativename'].': '.$found.' - Bad.');
 		//exit;
 
 		// Check string sql|set...'.$yyy->xxx   with xxx that is not 'escape', 'idate', .... It means we forget a db->escape when forging sql request.
 		$ok = true;
 		$matches = array();
+		$found = "";
 		preg_match_all('/(\$sql|SET\s|WHERE\s|INSERT\s|VALUES\s|VALUES\().+\s*\'\s*\.\s*\$(.........)/', $filecontent, $matches, PREG_SET_ORDER);
 		foreach ($matches as $key => $val) {
 			if (! in_array($val[2], array('this->db-', 'db->prefi', 'db->sanit', 'dbs->pref', 'dbs->sani', 'conf->ent', 'key : \'\')', 'key])."\')', 'excludefi', 'regexstri', ''))) {
+				$found = $val[0];
 				$ok = false;
 				var_dump($matches);
 				break;
@@ -393,7 +408,7 @@ class CodingPhpTest extends CommonClassTest
 			//if ($reg[0] != 'db') $ok=false;
 		}
 		//print __METHOD__." Result for checking we don't have non escaped string in sql requests for file ".$file."\n";
-			$this->assertTrue($ok, 'Found non escaped string in building of a sql request (case 3) in '.$file['relativename'].': '.$val[0].' - Bad.');
+		$this->assertTrue($ok, 'Found non escaped string in building of a sql request (case 3) in '.$file['relativename'].': '.$found.' - Bad.');
 		//exit;
 
 		// Checks with IN
@@ -577,9 +592,6 @@ class CodingPhpTest extends CommonClassTest
 		$this->assertTrue($ok, 'Found a CURDATE\(\) into code. Do not use this SQL method in file '.$file['relativename'].'. You must use the PHP function dol_now() instead.');
 	}
 
-		return;
-	}
-
 
 	/**
 	 * Verify that only known modules are used

test/phpunit/CommonClassTest.class.php

@@ -79,7 +79,9 @@ abstract class CommonClassTest extends TestCase
 		$this->savlangs = $langs;
 		$this->savdb = $db;
 
-		print __METHOD__." db->type=".$db->type." user->id=".$user->id;
+		if ((int) getenv('PHPUNIT_DEBUG') > 0) {
+			print get_called_class()." db->type=".$db->type." user->id=".$user->id;
+		}
 		//print " - db ".$db->db;
 		print PHP_EOL;
 	}
@@ -94,13 +96,8 @@ abstract class CommonClassTest extends TestCase
 		global $conf,$user,$langs,$db;
 		$db->begin(); // This is to have all actions inside a transaction even if test launched without suite.
 
-		if (!isModEnabled('agenda')) {
+		if ((int) getenv('PHPUNIT_DEBUG') > 0) {
-			print get_called_class()." module agenda must be enabled.".PHP_EOL;
+			print get_called_class()."::".__FUNCTION__.PHP_EOL;
-			die(1);
-		}
-
-		if (isset($_ENV['PHPUNIT_DEBUG'])) {
-			print get_called_class().PHP_EOL;
 		}
 	}
 
@@ -126,12 +123,24 @@ abstract class CommonClassTest extends TestCase
 		// Get the last line of the log
 		$last_lines = array_slice($lines, $first_line, $nbLinesToShow);
 
+		$failedTestMethod = $this->getName(false);
+		$className = get_called_class();
+
+		// Get the test method's reflection
+		$reflectionMethod = new ReflectionMethod($className, $failedTestMethod);
+
+		// Get the test method's data set
+		$argsText = $this->getDataSetAsString(true);
+
 		// Show log file
-		print PHP_EOL."----- Test fails. Show last ".$nbLinesToShow." lines of dolibarr.log file -----".PHP_EOL;
+		print PHP_EOL;
+		print "----- $className::$failedTestMethod failed - $argsText.".PHP_EOL;
+		print "Show last ".$nbLinesToShow." lines of dolibarr.log file -----".PHP_EOL;
 		foreach ($last_lines as $line) {
 			print $line . "<br>";
 		}
 		print PHP_EOL;
+		print "----- end of dolibarr.log for $className::$failedTestMethod".PHP_EOL;
 
 		parent::onNotSuccessfulTest($t);
 	}
@@ -150,10 +159,9 @@ abstract class CommonClassTest extends TestCase
 		$langs = $this->savlangs;
 		$db = $this->savdb;
 
-		if (isset($_ENV['PHPUNIT_DEBUG'])) {
+		if ((int) getenv('PHPUNIT_DEBUG') > 0) {
-			print get_called_class().PHP_EOL;
+			print get_called_class().'::'.$this->getName(false)."::".__FUNCTION__.PHP_EOL;
 		}
-		print __METHOD__."\n";
 		//print $db->getVersion()."\n";
 	}
 
@@ -164,8 +172,8 @@ abstract class CommonClassTest extends TestCase
 	 */
 	protected function tearDown(): void
 	{
-		if (isset($_ENV['PHPUNIT_DEBUG'])) {
+		if ((int) getenv('PHPUNIT_DEBUG') > 0) {
-			print get_called_class().PHP_EOL;
+			print get_called_class().'::'.$this->getName(false)."::".__FUNCTION__.PHP_EOL;
 		}
 	}
 
@@ -178,8 +186,8 @@ abstract class CommonClassTest extends TestCase
 	{
 		global $db;
 		$db->rollback();
-		if (isset($_ENV['PHPUNIT_DEBUG'])) {
+		if ((int) getenv('PHPUNIT_DEBUG') > 0) {
-			print get_called_class().PHP_EOL;
+			print get_called_class()."::".__FUNCTION__.PHP_EOL;
 		}
 	}