Issue #2791163 by alexpott, dawehner: Random automatic testing failures on SQLite with PHP 5.5

core/includes/bootstrap.inc

@@ -11,6 +11,7 @@
 use Drupal\Core\Logger\RfcLogLevel;
 use Drupal\Core\Render\Markup;
 use Drupal\Component\Render\MarkupInterface;
+use Drupal\Core\Test\TestDatabase;
 use Drupal\Core\Session\AccountInterface;
 use Drupal\Core\Site\Settings;
 use Drupal\Core\Utility\Error;
@@ -622,7 +623,7 @@ function drupal_valid_test_ua($new_prefix = NULL) {
   // string.
   $http_user_agent = isset($_SERVER['HTTP_USER_AGENT']) ? $_SERVER['HTTP_USER_AGENT'] : NULL;
   $user_agent = isset($_COOKIE['SIMPLETEST_USER_AGENT']) ? $_COOKIE['SIMPLETEST_USER_AGENT'] : $http_user_agent;
-  if (isset($user_agent) && preg_match("/^(simpletest\d+):(.+):(.+):(.+)$/", $user_agent, $matches)) {
+  if (isset($user_agent) && preg_match("/^simple(\w+\d+):(.+):(.+):(.+)$/", $user_agent, $matches)) {
     list(, $prefix, $time, $salt, $hmac) = $matches;
     $check_string = $prefix . ':' . $time . ':' . $salt;
     // Read the hash salt prepared by drupal_generate_test_ua().
@@ -630,7 +631,8 @@ function drupal_valid_test_ua($new_prefix = NULL) {
     // handlers are set up. While Drupal's error handling may be properly
     // configured on production sites, the server's PHP error_reporting may not.
     // Ensure that no information leaks on production sites.
-    $key_file = DRUPAL_ROOT . '/sites/simpletest/' . substr($prefix, 10) . '/.htkey';
+    $test_db = new TestDatabase($prefix);
+    $key_file = DRUPAL_ROOT . '/' . $test_db->getTestSitePath() . '/.htkey';
     if (!is_readable($key_file)) {
       header($_SERVER['SERVER_PROTOCOL'] . ' 403 Forbidden');
       exit;
@@ -661,7 +663,8 @@ function drupal_generate_test_ua($prefix) {
 
   if (!isset($key) || $last_prefix != $prefix) {
     $last_prefix = $prefix;
-    $key_file = DRUPAL_ROOT . '/sites/simpletest/' . substr($prefix, 10) . '/.htkey';
+    $test_db = new TestDatabase($prefix);
+    $key_file = DRUPAL_ROOT . '/' . $test_db->getTestSitePath() . '/.htkey';
     // When issuing an outbound HTTP client request from within an inbound test
     // request, then the outbound request has to use the same User-Agent header
     // as the inbound request. A newly generated private key for the same test
@@ -686,7 +689,7 @@ function drupal_generate_test_ua($prefix) {
   // Generate a moderately secure HMAC based on the database credentials.
   $salt = uniqid('', TRUE);
   $check_string = $prefix . ':' . time() . ':' . $salt;
-  return $check_string . ':' . Crypt::hmacBase64($check_string, $key);
+  return 'simple' . $check_string . ':' . Crypt::hmacBase64($check_string, $key);
 }
 
 /**

core/lib/Drupal/Core/Command/DbDumpCommand.php

@@ -33,7 +33,7 @@ class DbDumpCommand extends DbCommandBase {
    *
    * @var array
    */
-  protected $excludeTables = ['simpletest.+'];
+  protected $excludeTables = ['test[0-9]+'];
 
   /**
    * {@inheritdoc}

core/lib/Drupal/Core/DrupalKernel.php

@@ -18,6 +18,7 @@
 use Drupal\Core\Http\TrustedHostsRequestFactory;
 use Drupal\Core\Language\Language;
 use Drupal\Core\Site\Settings;
+use Drupal\Core\Test\TestDatabase;
 use Symfony\Cmf\Component\Routing\RouteObjectInterface;
 use Symfony\Component\ClassLoader\ApcClassLoader;
 use Symfony\Component\DependencyInjection\ContainerInterface;
@@ -365,7 +366,8 @@ public static function findSitePath(Request $request, $require_settings = TRUE,
 
     // Check for a simpletest override.
     if ($test_prefix = drupal_valid_test_ua()) {
-      return 'sites/simpletest/' . substr($test_prefix, 10);
+      $test_db = new TestDatabase($test_prefix);
+      return $test_db->getTestSitePath();
     }
 
     // Determine whether multi-site functionality is enabled.
@@ -944,6 +946,7 @@ public static function bootEnvironment($app_root = NULL) {
     // Indicate that code is operating in a test child site.
     if (!defined('DRUPAL_TEST_IN_CHILD_SITE')) {
       if ($test_prefix = drupal_valid_test_ua()) {
+        $test_db = new TestDatabase($test_prefix);
         // Only code that interfaces directly with tests should rely on this
         // constant; e.g., the error/exception handler conditionally adds further
         // error information into HTTP response headers that are consumed by
@@ -958,7 +961,7 @@ public static function bootEnvironment($app_root = NULL) {
 
         // Log fatal errors to the test site directory.
         ini_set('log_errors', 1);
-        ini_set('error_log', $app_root . '/sites/simpletest/' . substr($test_prefix, 10) . '/error.log');
+        ini_set('error_log', $app_root . '/' . $test_db->getTestSitePath() . '/error.log');
 
         // Ensure that a rewritten settings.php is used if opcache is on.
         ini_set('opcache.validate_timestamps', 'on');

core/lib/Drupal/Core/Test/TestDatabase.php

@@ -5,11 +5,31 @@
 use Drupal\Core\Database\ConnectionNotDefinedException;
 use Drupal\Core\Database\Database;
 
+// This class has a dependency on file_directory_os_temp().
+// @todo https://www.drupal.org/node/2794249 Remove once function moved to a
+//   class.
+require_once __DIR__ . '/../../../../includes/file.inc';
+
 /**
  * Provides helper methods for interacting with the Simpletest database.
  */
 class TestDatabase {
 
+  /**
+   * A random number used to ensure that test fixtures are unique to each test
+   * method.
+   *
+   * @var int
+   */
+  protected $lockId;
+
+  /**
+   * The test database prefix.
+   *
+   * @var string
+   */
+  protected $databasePrefix;
+
   /**
    * Returns the database connection to the site running Simpletest.
    *
@@ -41,4 +61,115 @@ public static function getConnection() {
     return $connection;
   }
 
+  /**
+   * TestDatabase constructor.
+   *
+   * @param string|null $db_prefix
+   *   If not provided a new test lock is generated.
+   *
+   * @throws \InvalidArgumentException
+   *   Thrown when $db_prefix does not match the regular expression.
+   */
+  public function __construct($db_prefix = NULL) {
+    if ($db_prefix === NULL) {
+      $this->lockId = $this->getTestLock();
+      $this->databasePrefix = 'test' . $this->lockId;
+    }
+    else {
+      $this->databasePrefix = $db_prefix;
+      // It is possible that we're running a test inside a test. In which case
+      // $db_prefix will be something like test12345678test90123456 and the
+      // generated lock ID for the running test method would be 90123456.
+      preg_match('/test(\d+)$/', $db_prefix, $matches);
+      if (!isset($matches[1])) {
+        throw new \InvalidArgumentException("Invalid database prefix: $db_prefix");
+      }
+      $this->lockId = $matches[1];
+    }
+  }
+
+  /**
+   * Gets the relative path to the test site directory.
+   *
+   * @return string
+   *   The relative path to the test site directory.
+   */
+  public function getTestSitePath() {
+    return 'sites/simpletest/' . $this->lockId;
+  }
+
+  /**
+   * Gets the test database prefix.
+   *
+   * @return string
+   *   The test database prefix.
+   */
+  public function getDatabasePrefix() {
+    return $this->databasePrefix;
+  }
+
+  /**
+   * Generates a unique lock ID for the test method.
+   *
+   * @return int
+   *   The unique lock ID for the test method.
+   */
+  protected function getTestLock() {
+    // Ensure that the generated lock ID is not in use, which may happen when
+    // tests are run concurrently.
+    do {
+      $lock_id = mt_rand(10000000, 99999999);
+      if (@symlink(__FILE__, $this->getLockFile($lock_id)) === FALSE) {
+        $lock_id = NULL;
+      }
+    } while ($lock_id === NULL);
+    return $lock_id;
+  }
+
+  /**
+   * Releases a test lock.
+   *
+   * This should only be called once the related test fixtures have been cleaned
+   * up.
+   */
+  public function releaseTestLock() {
+    $concurrency = getenv('RUN_TESTS_CONCURRENCY');
+    // If we're doing concurrent testing then ensure no dupes in the whole run.
+    if (!$concurrency || $concurrency == 1) {
+      unlink($this->getLockFile($this->lockId));
+    }
+  }
+
+  /**
+   * Releases all test locks.
+   *
+   * This should only be called once all the test fixtures have been cleaned up.
+   */
+  public static function releaseAllTestLocks() {
+    $tmp = file_directory_os_temp();
+    $dir = dir($tmp);
+    while (($entry = $dir->read()) !== FALSE) {
+      if ($entry === '.' || $entry === '..') {
+        continue;
+      }
+      $entry_path = $tmp . '/' . $entry;
+      if (preg_match('/^test_\d+/', $entry) && is_link($entry_path)) {
+        unlink($entry_path);
+      }
+    }
+  }
+
+  /**
+   * Gets the lock file path.
+   *
+   * @param int $lock_id
+   *   The test method lock ID.
+   *
+   * @return string
+   *   A file path to the symbolic link that prevents the lock ID being re-used.
+   */
+  protected function getLockFile($lock_id) {
+    return file_directory_os_temp() . '/test_' . $lock_id;
+  }
+
 }

core/modules/simpletest/simpletest.module

@@ -535,7 +535,8 @@ function simpletest_last_test_get($test_id) {
  *   Whether any fatal errors were found.
  */
 function simpletest_log_read($test_id, $database_prefix, $test_class) {
-  $log = DRUPAL_ROOT . '/sites/simpletest/' . substr($database_prefix, 10) . '/error.log';
+  $test_db = new TestDatabase($database_prefix);
+  $log = DRUPAL_ROOT . '/' . $test_db->getTestSitePath() . '/error.log';
   $found = FALSE;
   if (file_exists($log)) {
     foreach (file($log) as $line) {

core/modules/simpletest/src/TestBase.php

@@ -1108,14 +1108,9 @@ public function run(array $methods = array()) {
    * @see drupal_valid_test_ua()
    */
   private function prepareDatabasePrefix() {
-    // Ensure that the generated test site directory does not exist already,
-    // which may happen with a large amount of concurrent threads and
-    // long-running tests.
-    do {
-      $suffix = mt_rand(100000, 999999);
-      $this->siteDirectory = 'sites/simpletest/' . $suffix;
-      $this->databasePrefix = 'simpletest' . $suffix;
-    } while (is_dir(DRUPAL_ROOT . '/' . $this->siteDirectory));
+    $test_db = new TestDatabase();
+    $this->siteDirectory = $test_db->getTestSitePath();
+    $this->databasePrefix = $test_db->getDatabasePrefix();
 
     // As soon as the database prefix is set, the test might start to execute.
     // All assertions as well as the SimpleTest batch operations are associated
@@ -1371,6 +1366,10 @@ private function restoreEnvironment() {
     // Delete test site directory.
     file_unmanaged_delete_recursive($this->siteDirectory, array($this, 'filePreDeleteCallback'));
 
+    // Release the test lock.
+    $test_db = new TestDatabase($test_prefix);
+    $test_db->releaseTestLock();
+
     // Restore original database connection.
     Database::removeConnection('default');
     Database::renameConnection('simpletest_original_default', 'default');

core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php

@@ -89,7 +89,7 @@ public function testUserAgentValidation() {
     $HTTP_path = $system_path . '/tests/http.php/user/login';
     $https_path = $system_path . '/tests/https.php/user/login';
     // Generate a valid simpletest User-Agent to pass validation.
-    $this->assertTrue(preg_match('/simpletest\d+/', $this->databasePrefix, $matches), 'Database prefix contains simpletest prefix.');
+    $this->assertTrue(preg_match('/test\d+/', $this->databasePrefix, $matches), 'Database prefix contains test prefix.');
     $test_ua = drupal_generate_test_ua($matches[0]);
     $this->additionalCurlOptions = array(CURLOPT_USERAGENT => $test_ua);
 

core/modules/simpletest/src/Tests/SimpleTestTest.php

@@ -3,6 +3,7 @@
 namespace Drupal\simpletest\Tests;
 
 use Drupal\Component\Utility\Crypt;
+use Drupal\Core\Test\TestDatabase;
 use Drupal\simpletest\WebTestBase;
 
 /**
@@ -154,7 +155,8 @@ function stubTest() {
     // test in a test since the prefix has changed.
     // @see \Drupal\Core\Test\HttpClientMiddleware\TestHttpClientMiddleware::onBeforeSendRequest()
     // @see drupal_generate_test_ua();
-    $key_file = DRUPAL_ROOT . '/sites/simpletest/' . substr($this->databasePrefix, 10) . '/.htkey';
+    $test_db = new TestDatabase($this->databasePrefix);
+    $key_file = DRUPAL_ROOT . '/' . $test_db->getTestSitePath() . '/.htkey';
     $private_key = Crypt::randomBytesBase64(55);
     $site_path = $this->container->get('site.path');
     file_put_contents($key_file, $private_key);

core/modules/simpletest/src/WebTestBase.php

@@ -1162,9 +1162,7 @@ protected function curlInitialize() {
     }
     // We set the user agent header on each request so as to use the current
     // time and a new uniqid.
-    if (preg_match('/simpletest\d+/', $this->databasePrefix, $matches)) {
-      curl_setopt($this->curlHandle, CURLOPT_USERAGENT, drupal_generate_test_ua($matches[0]));
-    }
+    curl_setopt($this->curlHandle, CURLOPT_USERAGENT, drupal_generate_test_ua($this->databasePrefix));
   }
 
   /**

core/scripts/run-tests.sh

@@ -11,6 +11,7 @@
 use Drupal\Core\Database\Database;
 use Drupal\Core\Site\Settings;
 use Drupal\Core\StreamWrapper\PublicStream;
+use Drupal\Core\Test\TestDatabase;
 use Drupal\Core\Test\TestRunnerKernel;
 use Drupal\simpletest\Form\SimpletestResultsForm;
 use Drupal\simpletest\TestBase;
@@ -124,6 +125,12 @@
 // Stop the timer.
 simpletest_script_reporter_timer_stop();
 
+// Ensure all test locks are released once finished. If tests are run with a
+// concurrency of 1 the each test will clean up it's own lock. Test locks are
+// not released if using a higher concurrency to ensure each test method has
+// unique fixtures.
+TestDatabase::releaseAllTestLocks();
+
 // Display results before database is cleared.
 if ($args['browser']) {
   simpletest_script_open_browser();
@@ -447,6 +454,10 @@ function simpletest_script_init() {
   $_SERVER['PHP_SELF'] = $path . '/index.php';
   $_SERVER['HTTP_USER_AGENT'] = 'Drupal command line';
 
+  if ($args['concurrency'] > 1) {
+    putenv('RUN_TESTS_CONCURRENCY=' . $args['concurrency']);
+  }
+
   if (!empty($_SERVER['HTTPS']) && $_SERVER['HTTPS'] == 'on') {
     // Ensure that any and all environment variables are changed to https://.
     foreach ($_SERVER as $key => $value) {
@@ -665,7 +676,8 @@ function simpletest_script_execute_batch($test_classes) {
           );
           if ($args['die-on-fail']) {
             list($db_prefix) = simpletest_last_test_get($child['test_id']);
-            $test_directory = 'sites/simpletest/' . substr($db_prefix, 10);
+            $test_db = new TestDatabase($db_prefix);
+            $test_directory = $test_db->getTestSitePath();
             echo 'Simpletest database and files kept and test exited immediately on fail so should be reproducible if you change settings.php to use the database prefix ' . $db_prefix . ' and config directories in ' . $test_directory . "\n";
             $args['keep-results'] = TRUE;
             // Exit repeat loop immediately.
@@ -841,7 +853,8 @@ function simpletest_script_cleanup($test_id, $test_class, $exitcode) {
 
   // Check whether a test site directory was setup already.
   // @see \Drupal\simpletest\TestBase::prepareEnvironment()
-  $test_directory = DRUPAL_ROOT . '/sites/simpletest/' . substr($db_prefix, 10);
+  $test_db = new TestDatabase($db_prefix);
+  $test_directory = DRUPAL_ROOT . '/' . $test_db->getTestSitePath();
   if (is_dir($test_directory)) {
     // Output the error_log.
     if (is_file($test_directory . '/error.log')) {

core/tests/Drupal/KernelTests/KernelTestBase.php

@@ -15,6 +15,7 @@
 use Drupal\Core\Extension\ExtensionDiscovery;
 use Drupal\Core\Language\Language;
 use Drupal\Core\Site\Settings;
+use Drupal\Core\Test\TestDatabase;
 use Drupal\simpletest\AssertContentTrait;
 use Drupal\simpletest\AssertHelperTrait;
 use Drupal\Tests\ConfigTestTrait;
@@ -251,19 +252,14 @@ protected function bootEnvironment() {
     require_once $this->root . '/core/includes/bootstrap.inc';
 
     // Set up virtual filesystem.
-    // Ensure that the generated test site directory does not exist already,
-    // which may happen with a large amount of concurrent threads and
-    // long-running tests.
-    do {
-      $suffix = mt_rand(100000, 999999);
-      $this->siteDirectory = 'sites/simpletest/' . $suffix;
-      $this->databasePrefix = 'simpletest' . $suffix;
-    } while (is_dir($this->root . '/' . $this->siteDirectory));
+    Database::addConnectionInfo('default', 'test-runner', $this->getDatabaseConnectionInfo()['default']);
+    $test_db = new TestDatabase();
+    $this->siteDirectory = $test_db->getTestSitePath();
 
     // Ensure that all code that relies on drupal_valid_test_ua() can still be
     // safely executed. This primarily affects the (test) site directory
     // resolution (used by e.g. LocalStream and PhpStorage).
-    $this->databasePrefix = 'simpletest' . $suffix;
+    $this->databasePrefix = $test_db->getDatabasePrefix();
     drupal_valid_test_ua($this->databasePrefix);
 
     $settings = array(
@@ -287,15 +283,12 @@ protected function bootEnvironment() {
    * Sets up the filesystem, so things like the file directory.
    */
   protected function setUpFilesystem() {
-    $suffix = str_replace('simpletest', '', $this->databasePrefix);
-    $this->vfsRoot = vfsStream::setup('root', NULL, array(
-      'sites' => array(
-        'simpletest' => array(
-          $suffix => array(),
-        ),
-      ),
-    ));
-    $this->siteDirectory = vfsStream::url('root/sites/simpletest/' . $suffix);
+    $test_db = new TestDatabase($this->databasePrefix);
+    $test_site_path = $test_db->getTestSitePath();
+
+    $this->vfsRoot = vfsStream::setup('root');
+    $this->vfsRoot->addChild(vfsStream::newDirectory($test_site_path));
+    $this->siteDirectory = vfsStream::url('root/' . $test_site_path);
 
     mkdir($this->siteDirectory . '/files', 0775);
     mkdir($this->siteDirectory . '/files/config/' . CONFIG_SYNC_DIRECTORY, 0775, TRUE);
@@ -708,6 +701,10 @@ protected function tearDown() {
     $this->vfsRoot = NULL;
     $this->configImporter = NULL;
 
+    // Release the prefix.
+    $test_db = new TestDatabase($test_prefix);
+    $test_db->releaseTestLock();
+
     // Free up memory: Custom test class properties.
     // Note: Private properties cannot be cleaned up.
     $rc = new \ReflectionClass(__CLASS__);

core/tests/Drupal/KernelTests/KernelTestBaseTest.php

@@ -25,13 +25,13 @@ public function testSetUpBeforeClass() {
    * @covers ::bootEnvironment
    */
   public function testBootEnvironment() {
-    $this->assertRegExp('/^simpletest\d{6}$/', $this->databasePrefix);
+    $this->assertRegExp('/^test\d{8}$/', $this->databasePrefix);
     $this->assertStringStartsWith('vfs://root/sites/simpletest/', $this->siteDirectory);
     $this->assertEquals(array(
       'root' => array(
         'sites' => array(
           'simpletest' => array(
-            substr($this->databasePrefix, 10) => array(
+            substr($this->databasePrefix, 4) => array(
               'files' => array(
                 'config' => array(
                   'sync' => array(),

core/tests/Drupal/Tests/BrowserTestBase.php

@@ -541,6 +541,10 @@ protected function cleanupEnvironment() {
 
     // Delete test site directory.
     file_unmanaged_delete_recursive($this->siteDirectory, array($this, 'filePreDeleteCallback'));
+
+    // Release the prefix.
+    $test_db = new TestDatabase($test_prefix);
+    $test_db->releaseTestLock();
   }
 
   /**
@@ -1201,14 +1205,9 @@ protected function installParameters() {
    * @see BrowserTestBase::prepareEnvironment()
    */
   private function prepareDatabasePrefix() {
-    // Ensure that the generated test site directory does not exist already,
-    // which may happen with a large amount of concurrent threads and
-    // long-running tests.
-    do {
-      $suffix = mt_rand(100000, 999999);
-      $this->siteDirectory = 'sites/simpletest/' . $suffix;
-      $this->databasePrefix = 'simpletest' . $suffix;
-    } while (is_dir(DRUPAL_ROOT . '/' . $this->siteDirectory));
+    $test_db = new TestDatabase();
+    $this->siteDirectory = $test_db->getTestSitePath();
+    $this->databasePrefix = $test_db->getDatabasePrefix();
   }
 
   /**

core/tests/Drupal/Tests/Core/Test/TestDatabaseTest.php

+<?php
+
+namespace Drupal\Tests\Core\Test;
+
+use Drupal\Core\Test\TestDatabase;
+use Drupal\Tests\UnitTestCase;
+
+/**
+ * @coversDefaultClass \Drupal\Core\Test\TestDatabase
+ * @group Template
+ */
+class TestDatabaseTest extends UnitTestCase {
+
+  /**
+   * @covers ::__construct
+   */
+  public function testConstructorException() {
+    $this->setExpectedException(\InvalidArgumentException::class, "Invalid database prefix: blah1253");
+    new TestDatabase('blah1253');
+  }
+
+  /**
+   * @covers ::__construct
+   * @covers ::getDatabasePrefix
+   * @covers ::getTestSitePath
+   *
+   * @dataProvider providerTestConstructor
+   */
+  public function testConstructor($db_prefix, $expected_db_prefix, $expected_site_path) {
+    $test_db = new TestDatabase($db_prefix);
+    $this->assertEquals($expected_db_prefix, $test_db->getDatabasePrefix());
+    $this->assertEquals($expected_site_path, $test_db->getTestSitePath());
+  }
+
+  /**
+   * Data provider for self::testConstructor()
+   */
+  public function providerTestConstructor() {
+    return [
+      ['test1234', 'test1234', 'sites/simpletest/1234'],
+      ['test123456test234567', 'test123456test234567', 'sites/simpletest/234567']
+    ];
+  }
+
+}