test: Correctly check if database was used in a test

Signed-off-by: provokateurin <kate@provokateurin.de>

Author: provokateurin

tests/lib/TestCase.php

@@ -10,6 +10,7 @@
 
 use OC\App\AppStore\Fetcher\AppFetcher;
 use OC\Command\QueueBus;
+use OC\DB\Connection;
 use OC\Files\AppData\Factory;
 use OC\Files\Cache\Storage;
 use OC\Files\Config\MountProviderCollection;
@@ -42,23 +43,13 @@
 abstract class TestCase extends \PHPUnit\Framework\TestCase {
 	private QueueBus $commandBus;
 
-	/** @psalm-suppress ImpureStaticProperty For tests it's not an issue */
-	protected static ?IDBConnection $realDatabase = null;
 	/** @psalm-suppress ImpureStaticProperty */
-	private static bool $wasDatabaseAllowed = false;
 	protected array $services = [];
 
 	#[\Override]
 	protected function onNotSuccessfulTest(\Throwable $t): never {
 		$this->restoreAllServices();
 
-		// restore database connection
-		if (!$this->IsDatabaseAccessAllowed()) {
-			\OC::$server->registerService(IDBConnection::class, function () {
-				return self::$realDatabase;
-			});
-		}
-
 		parent::onNotSuccessfulTest($t);
 	}
 
@@ -134,38 +125,37 @@ protected function setUp(): void {
 		$this->overwriteService('AsyncCommandBus', $this->commandBus);
 		$this->overwriteService(IBus::class, $this->commandBus);
 
-		// detect database access
-		self::$wasDatabaseAllowed = true;
-		if (!$this->IsDatabaseAccessAllowed()) {
-			self::$wasDatabaseAllowed = false;
-			if (is_null(self::$realDatabase)) {
-				self::$realDatabase = Server::get(IDBConnection::class);
-			}
-			/** @psalm-suppress InternalMethod */
-			\OC::$server->registerService(IDBConnection::class, function (): void {
-				$this->fail('Your test case is not allowed to access the database.');
-			});
-		}
-
 		$traits = $this->getTestTraits();
 		foreach ($traits as $trait) {
 			$methodName = 'setUp' . basename(str_replace('\\', '/', $trait));
 			if (method_exists($this, $methodName)) {
 				call_user_func([$this, $methodName]);
 			}
 		}
+
+		// Something outside of our test might have done something with the database already, but we ignore it.
+		$this->resetDatabaseStats();
 	}
 
 	#[\Override]
 	protected function tearDown(): void {
 		$this->restoreAllServices();
 
-		// restore database connection
-		if (!$this->IsDatabaseAccessAllowed()) {
-			/** @psalm-suppress InternalMethod */
-			\OC::$server->registerService(IDBConnection::class, function () {
-				return self::$realDatabase;
-			});
+		$databaseAllowed = $this->IsDatabaseAccessAllowed();
+		$databaseUsed = $this->wasDatabaseUsed();
+		$this->resetDatabaseStats();
+		if ($databaseUsed && !$databaseAllowed) {
+			$this->fail('The database was used, but it was not allowed');
+		}
+		// Some test methods using a data provider generate cases that both use and not use the DB
+		// Therefore we can't enable this check all the time.
+		// It's useful for manually checking if all tests are using the correct group though.
+		// 1. DB_UNUSED=1 ./autotest.sh sqlite | grep -B 1 "The database was not used, but it was allowed" | grep "::" | cut -d ":" -f 1 | cut -d " " -f 2 | sort -u
+		// 2. Remove the Group attribute from all classes from the output
+		// 3. ./autotest.sh sqlite | grep -B 1 "The database was used, but it was not allowed" | grep "::" | cut -d " " -f 2 | sort -u
+		// 4. Add the Group attribute to all methods from the output
+		if (!$databaseUsed && $databaseAllowed && getenv('DB_UNUSED') !== false) {
+			$this->fail('The database was not used, but it was allowed');
 		}
 
 		// further cleanup
@@ -298,26 +288,17 @@ public function filterClassMethods(string $className, array $filterMethods): arr
 
 	#[\Override]
 	public static function tearDownAfterClass(): void {
-		if (!self::$wasDatabaseAllowed && self::$realDatabase !== null) {
-			// in case an error is thrown in a test, PHPUnit jumps straight to tearDownAfterClass,
-			// so we need the database again
-			\OC::$server->registerService(IDBConnection::class, function () {
-				return self::$realDatabase;
-			});
-		}
 		$dataDir = Server::get(IConfig::class)->getSystemValueString('datadirectory', \OC::$SERVERROOT . '/data-autotest');
-		if (self::$wasDatabaseAllowed && Server::get(IDBConnection::class)) {
-			$db = Server::get(IDBConnection::class);
-			if ($db->inTransaction()) {
-				$db->rollBack();
-				throw new \Exception('There was a transaction still in progress and needed to be rolled back. Please fix this in your test.');
-			}
-			$queryBuilder = $db->getQueryBuilder();
-
-			self::tearDownAfterClassCleanShares($queryBuilder);
-			self::tearDownAfterClassCleanStorages($queryBuilder);
-			self::tearDownAfterClassCleanFileCache($queryBuilder);
+		$db = Server::get(IDBConnection::class);
+		if ($db->inTransaction()) {
+			$db->rollBack();
+			self::fail('There was a transaction still in progress and needed to be rolled back. Please fix this in your test.');
 		}
+
+		$queryBuilder = $db->getQueryBuilder();
+		self::tearDownAfterClassCleanShares($queryBuilder);
+		self::tearDownAfterClassCleanStorages($queryBuilder);
+		self::tearDownAfterClassCleanFileCache($queryBuilder);
 		self::tearDownAfterClassCleanStrayDataFiles($dataDir);
 		self::tearDownAfterClassCleanStrayHooks();
 		self::tearDownAfterClassCleanStrayLocks();
@@ -556,4 +537,16 @@ protected function IsDatabaseAccessAllowed(): bool {
 		$annotations = $this->getGroupAnnotations();
 		return in_array('DB', $annotations) || in_array('SLOWDB', $annotations);
 	}
+
+	private function wasDatabaseUsed(): bool {
+		$connection = Server::get(Connection::class);
+		$databaseStats = $connection->getStats();
+		return $databaseStats['built'] > 0 || $databaseStats['executed'] > 0;
+	}
+
+	private function resetDatabaseStats(): void {
+		$connection = Server::get(Connection::class);
+		self::invokePrivate($connection, 'queriesBuilt', [0]);
+		self::invokePrivate($connection, 'queriesExecuted', [0]);
+	}
 }