From a5a672b1a5fddffd5d03f26ecae743768e7c8e9c Mon Sep 17 00:00:00 2001
From: Timo Tijhof <krinklemail@gmail.com>
Date: Mon, 15 Jul 2019 00:49:30 +0100
Subject: [PATCH] resourceloader: Speed up dependency checks in
 structure/ResourcesTest

Stats from wmf-quibble-core-vendor-mysql-php72-docker builds.
Before:
* testIllegalDependencies (+21ms)
* testMissingDependencies (+254ms)
After:
* testValidDependencies (+17ms)

Bug: T225730
Change-Id: Idf760a27c7ad16d4838ae82e7895b659934fbf93
---
 tests/phpunit/structure/ResourcesTest.php | 66 ++++++++++-------------
 1 file changed, 28 insertions(+), 38 deletions(-)

diff --git a/tests/phpunit/structure/ResourcesTest.php b/tests/phpunit/structure/ResourcesTest.php
index a762884886..4f9664f4ca 100644
--- a/tests/phpunit/structure/ResourcesTest.php
+++ b/tests/phpunit/structure/ResourcesTest.php
@@ -45,53 +45,43 @@ class ResourcesTest extends MediaWikiTestCase {
 	}
 
 	/**
-	 * Verify that nothing depends on "startup".
+	 * Verify that all modules specified as dependencies of other modules actually
+	 * exist and are not illegal.
 	 *
-	 * Depending on it is unsupported as it cannot be loaded by the client.
-	 *
-	 * @todo Modules can dynamically choose dependencies based on context. This method does not
-	 * test such dependencies. The same goes for testMissingDependencies() and
-	 * testUnsatisfiableDependencies().
+	 * @todo Modules can dynamically choose dependencies based on context. This method
+	 * does not find all such variations. The same applies to testUnsatisfiableDependencies().
 	 */
-	public function testIllegalDependencies() {
+	public function testValidDependencies() {
 		$data = self::getAllModules();
-
-		$illegalDeps = [];
-		foreach ( $data['modules'] as $moduleName => $module ) {
-			if ( $module instanceof ResourceLoaderStartUpModule ) {
-				$illegalDeps[] = $moduleName;
-			}
-		}
-
-		/** @var ResourceLoaderModule $module */
-		foreach ( $data['modules'] as $moduleName => $module ) {
-			foreach ( $illegalDeps as $illegalDep ) {
-				$this->assertNotContains(
-					$illegalDep,
-					$module->getDependencies( $data['context'] ),
-					"Module '$moduleName' must not depend on '$illegalDep'"
-				);
-			}
-		}
-	}
-
-	/**
-	 * Verify that all modules specified as dependencies of other modules actually exist.
-	 */
-	public function testMissingDependencies() {
-		$data = self::getAllModules();
-		$validDeps = array_keys( $data['modules'] );
+		$knownDeps = array_keys( $data['modules'] );
+		$illegalDeps = [ 'startup' ];
+
+		// Avoid an assert for each module to keep the test fast.
+		// Instead, perform a single assertion against everything at once.
+		// When all is good, actual/expected are both empty arrays.
+		// When we find issues, add the violations to 'actual' and add an empty
+		// key to 'expected'. These keys in expected are because the PHPUnit diff
+		// (as of 6.5) only goes one level deep.
+		$actualUnknown = [];
+		$expectedUnknown = [];
+		$actualIllegal = [];
+		$expectedIllegal = [];
 
 		/** @var ResourceLoaderModule $module */
 		foreach ( $data['modules'] as $moduleName => $module ) {
 			foreach ( $module->getDependencies( $data['context'] ) as $dep ) {
-				$this->assertContains(
-					$dep,
-					$validDeps,
-					"The module '$dep' required by '$moduleName' must exist"
-				);
+				if ( !in_array( $dep, $knownDeps, true ) ) {
+					$actualUnknown[$moduleName][] = $dep;
+					$expectedUnknown[$moduleName] = [];
+				}
+				if ( in_array( $dep, $illegalDeps, true ) ) {
+					$actualIllegal[$moduleName][] = $dep;
+					$expectedIllegal[$moduleName] = [];
+				}
 			}
 		}
+		$this->assertEquals( $expectedUnknown, $actualUnknown, 'Dependencies that do not exist' );
+		$this->assertEquals( $expectedIllegal, $actualIllegal, 'Dependencies that are not legal' );
 	}
 
 	/**
-- 
2.20.1