Merge "phpunit: Add error_reporting/AtEase check to MediaWikiUnitTestCase"

.phpcs.xml

@@ -180,7 +180,29 @@
 		<exclude-pattern>*/tests/phpunit/maintenance/*\.php</exclude-pattern>
 		<exclude-pattern>*/tests/phpunit/integration/includes/GlobalFunctions/*\.php</exclude-pattern>
 	</rule>
+	<rule ref="Generic.PHP.NoSilencedErrors.Discouraged">
+		<!--
+		Our normal policy of using Wikimedia\AtEase does not always make sense tests.
 
+		AtEase cannot be cleanly used in tests that also use expectException() as
+		the restoreWarnings() call would never be reached:
+
+			$this->expectException( PasswordError::class );
+			AtEase::suppressWarnings();
+			$password->crypt( 'whatever' );
+			AtEase::restoreWarnings();
+
+		The above will stop at crypt(), as expected, and leave AtEase in a dirty
+		state for unrelated tests.
+
+		TODO: Stop using PHPUnit TestCase directly. Require with a structure test
+		or with a high-level check in our run() hook, that all test cases use either
+		MediaWikiUnitTestCase or MediaWikiIntegrationTestCase. Otherwise the check
+		in MediaWikiTestCaseTrait can still be bypassed and cause a random failures.
+
+		-->
+		<exclude-pattern>*/tests/*\.php</exclude-pattern>
+	</rule>
 	<rule ref="Generic.Files.OneObjectStructurePerFile.MultipleFound">
 		<!--
 			Whitelist existing violations, but enable the sniff to prevent

tests/phpunit/MediaWikiIntegrationTestCase.php

@@ -74,13 +74,6 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase {
 	private static $dbSetup = false;
 	private static $oldTablePrefix = '';
 
-	/**
-	 * Original value of PHP's error_reporting setting.
-	 *
-	 * @var int
-	 */
-	private $phpErrorLevel;
-
 	/**
 	 * Holds the paths of temporary files/directories created through getNewTempFile,
 	 * and getNewTempDirectory
@@ -520,8 +513,6 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase {
 	 * @before
 	 */
 	protected function mediaWikiSetUp() {
-		$this->phpErrorLevel = intval( ini_get( 'error_reporting' ) );
-
 		$reflection = new ReflectionClass( $this );
 		// TODO: Eventually we should assert for test presence in /integration/
 		if ( strpos( $reflection->getFilename(), '/unit/' ) !== false ) {
@@ -647,19 +638,6 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase {
 		MediaWiki\Session\SessionManager::resetCache();
 		MediaWiki\Auth\AuthManager::resetCache();
 
-		$phpErrorLevel = intval( ini_get( 'error_reporting' ) );
-
-		if ( $phpErrorLevel !== $this->phpErrorLevel ) {
-			ini_set( 'error_reporting', $this->phpErrorLevel );
-
-			$oldVal = self::formatErrorLevel( $this->phpErrorLevel );
-			$newVal = self::formatErrorLevel( $phpErrorLevel );
-			$message = "PHP error_reporting setting was left dirty: "
-				. "was $oldVal before test, $newVal after test!";
-
-			$this->fail( $message );
-		}
-
 		// If anything faked the time, reset it
 		ConvertibleTimestamp::setFakeTime( false );
 	}

tests/phpunit/MediaWikiTestCaseTrait.php

@@ -7,6 +7,9 @@ use PHPUnit\Framework\MockObject\MockObject;
  * For code common to both MediaWikiUnitTestCase and MediaWikiIntegrationTestCase.
  */
 trait MediaWikiTestCaseTrait {
+	/** @var int|null */
+	private $originalPhpErrorFilter;
+
 	/**
 	 * Returns a PHPUnit constraint that matches anything other than a fixed set of values. This can
 	 * be used to whitelist values, e.g.
@@ -149,4 +152,25 @@ trait MediaWikiTestCaseTrait {
 			}
 		);
 	}
+
+	/**
+	 * @before
+	 */
+	protected function phpErrorFilterSetUp() {
+		$this->originalPhpErrorFilter = intval( ini_get( 'error_reporting' ) );
+	}
+
+	/**
+	 * @after
+	 */
+	protected function phpErrorFilterTearDown() {
+		$phpErrorFilter = intval( ini_get( 'error_reporting' ) );
+
+		if ( $phpErrorFilter !== $this->originalPhpErrorFilter ) {
+			ini_set( 'error_reporting', $this->originalPhpErrorFilter );
+			$message = "PHP error_reporting setting found dirty."
+				. " Did you forget AtEase::restoreWarnings?";
+			$this->fail( $message );
+		}
+	}
 }

tests/phpunit/unit/includes/password/Pbkdf2PasswordTest.php

@@ -1,7 +1,5 @@
 <?php
 
-use Wikimedia\AtEase\AtEase;
-
 /**
  * @group large
  * @covers Pbkdf2Password
@@ -41,8 +39,6 @@ class Pbkdf2PasswordTest extends PasswordTestCase {
 		);
 		$this->expectException( PasswordError::class );
 		$this->expectExceptionMessage( 'Error when hashing password.' );
-		AtEase::suppressWarnings();
+		@$password->crypt( 'whatever' );
-		$password->crypt( 'whatever' );
-		AtEase::restoreWarnings();
 	}
 }