Improve performance of functional tests by caching Drupal installations

Patch #48 works with Drupal 9.5

Run tests of my module without this patch: Time: 01:27.310

Run tests of my module with this patch: Time: 00:36.736

tyler36, you need to add BROWSERTEST_CACHE_DB=1 before phpunit exec. In my case this is

fin exec BROWSERTEST_CACHE_DB=1 vendor/bin/phpunit --testsuite=my_module

@ivnish Thank you for pointing out the BROWSERTEST_CACHE_DB requirement.

Tested #48 on Drupal 10.0.9 one existing project with mixture of unit, kernel and functional tests (32 tests, 280 assertions).
Time went from 02:28 => 1:13. Multiple runs averaged about 50 second saved.

I think the environmental variable should be added to core/phpunit.xml.dist. This allows it to be the "default" setting.

  <php>
   ...
    <env name="BROWSERTEST_CACHE_DB" value="1"/>
  </php>

#55 is a good suggestion. I'd add it to the list of changes requested in #35

I have a lot of bugs with DB cache. For example "unknown schema" and "route not found". Be careful

This is an updated version of 2900208-48.patch which is now compatible with Drupal 10.1.x version.

This is an updated version of 2900208-59.patch with whitespaces removed, compatible with Drupal 10.1.x version.

Fixed failures in #60.

I manually compared patch #48 and patch #59 and #60. Patches #59 and #60 are garbage patches without any changes. Only code style was broken

Patch #61 added one new change

  /**
   * The "#1" admin user.
   *
   * @var \Drupal\TestSite\Commands\TestSiteInstallCommand
   */
  protected $dumpFile;

It would be useful to see if this makes a difference on gitlab ci. We'd need an MR, with the environment variable shortcutted (or set in the gitlab YAML).

gitlab would make sense. and #60 would be good to explain why you added a new change.

Dave Reid → made their first commit to this issue’s fork.

I found an issue using the latest version on 10.2 where $connection_info['default']['driver'] is now Drupal\mysql\Driver\Database\mysql instead of mysql. Opened an official MR for better visibility and GitLab CI testing.

So far, I've been able to fix the concurrency issues and improve performance by reworking the methods as follows:

  /**
   * Installs Drupal into the Simpletest site.
   */
  public function installDrupal() {
    if (getenv('BROWSERTEST_CACHE_DB')) {
      $this->initDumpFile();

      $lock_file = $this->dumpFile . '.lock';

      $this->runWithLockFile($lock_file, function () use (&$install_from_dump) {
        if (file_exists($this->dumpFile)) {
          $install_from_dump = TRUE;
        }
        else {
          $this->installDrupalFromProfile();
          $this->dumpDatabase();

          $install_from_dump = FALSE;
        }
      });

      // The check for the dump file happens inside the critical section to
      // ensure it blocks while a dump is being created, but the installation
      // happens outside the critical section so that multiple tests can use the
      // same dump file concurrently.
      if ($install_from_dump) {
        $this->installDrupalFromDump();
      }
    }
    else {
      $this->installDrupalFromProfile();
    }
  }

  /**
   * Determines a proper file name for SQL dump.
   */
  protected function initDumpFile() {
    $class = get_class($this);
    $modules = [];
    while ($class) {
      if (property_exists($class, 'modules')) {
        $modules = array_merge($modules, $class::$modules);
      }
      $class = get_parent_class($class);
    }
    sort($modules);
    array_unique($modules);
    $temp_dir = sys_get_temp_dir();
    $cache_dir = getenv('BROWSERTEST_CACHE_DIR') ?: $temp_dir . '/test_dumps/' . \Drupal::VERSION;
    $this->dumpFile = $cache_dir . '/' . md5(implode('-', $modules)) . '.sql';

    // Create a folder to contain the test database dumps, if it does not
    // already exist. Only one test is allowed to create this at a time.
    $lock_file = sprintf("%s/test_dumps-%s.lock", $temp_dir, \Drupal::VERSION);
    $this->runWithLockFile($lock_file, function () use ($cache_dir) {
      if (!is_dir($cache_dir)) {
        mkdir($cache_dir, 0777, TRUE);
      }
    });
  }

  /**
   * Acquires an exclusive lock on the specified file and runs the given code.
   *
   * This is used to ensure that only one test is executing the same code at the
   * same time on the same runner. The lock is released after the critical code
   * returns.
   *
   * @param string $lock_file_path
   *   The path to the lock file.
   * @param callable $critical_code
   *   The code to invoke only once the lock is obtained.
   */
  private function runWithLockFile(string $lock_file_path, callable $critical_code) {
    $locked = FALSE;
    $lock_handle = fopen($lock_file_path, 'w');
    try {
      if (($lock_handle !== FALSE) && flock($lock_handle, LOCK_EX)) {
        $locked = TRUE;

        $critical_code();
      }
      else {
        $this->fail('Failed to acquire lock: ' . $lock_file_path);
      }
    }
    finally {
      // Delete the lock file before unlocking it to ensure we don't delete a
      // lock created by another process.
      if (file_exists($lock_file_path)) {
        @unlink($lock_file_path);
      }
      if ($locked) {
        @flock($lock_handle, LOCK_UN);
      }
      if ($lock_handle !== FALSE) {
        @fclose($lock_handle);
      }
    }
  }

An interdiff (against #48) is attached for the changes above, but I was curious about feedback on this approach before I apply them to the MR.

If a full install + module installs are no longer needed, but just a drush site:install --existing-config, then that would already speed up things considerably.

Which means that 🐛 Configuration being imported by the ConfigImporter sometimes has stale original data Fixed (which just landed) should also help with this.

The provided patches don't consider install profile and default theme.
Here is a small enhancement to the patch.

Let's use MR, not patches

@GuyPaddock can you apped MR with your changes #69?

There is a call-out on slack asking for help to test this
https://drupal.slack.com/archives/C223PR743/p1723469165431189

Can someone update the issue summary here to explain how you would like this tested, how to run it, what info you want, with & without the change. Thanks.

I will update issue summary

btw Sqlite just needs to copy DB-file after install

Would be good to see how this affects test run times once the MR is green but also we should check it against the times on 📌 Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller Active which significantly improves performance of the installer.

MR actually appears to be red

I have rebase the MR, it was 110 commits behind.
The MR is red because of a couple of PHPStan failures

----- -------------------------------------------------------------------------- 
  Line   core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php (in context of     
         class Drupal\TestSite\Commands\TestSiteInstallCommand)                    
 ------ -------------------------------------------------------------------------- 
  830    Access to an undefined property                                           
         Drupal\TestSite\Commands\TestSiteInstallCommand::$dumpFile.               
         💡 Learn more:                                                            
            https://phpstan.org/blog/solving-phpstan-access-to-undefined-property  
  852    Access to an undefined property                                           
         Drupal\TestSite\Commands\TestSiteInstallCommand::$dumpFile.               
         💡 Learn more:                                                            
            https://phpstan.org/blog/solving-phpstan-access-to-undefined-property  

These are in the new functions dumpDatabase() and restoreDatabase(), but the critical bit I think is in context of class Drupal\TestSite\Commands\TestSiteInstallCommand as that uses this trait, see
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/tests/Drupal/...

I moved the definition of dumpFile from BrowserTestBase to FunctionalTestSetupTrait.

The MR pipeline now runs green :-)

I am testing this locally, but I get

PHPUnit\Framework\Exception: mysqldump: [Warning] Using a password on the command line interface can be insecure.

and this halts the tests and gives "This test did not perform any assertions"
What's the bet way around this? There are multipe ways to do it, maybe we want to adust the two new mysql commands to use a file for the credentials?

> [Warning] Using a password on the command line interface can be insecure.

Last time I looked, drush created a temporary file using tmpfile() with the login data and passed that to mysqdump / mysql via --defaults-extra-file. Given that the temporary file is deleted by PHP at the end of the execution of the process and we're only using this during testing and not in production, this might be sufficiently secure, at least more secure than passing this on the command line or using environment variables (on the other hand, I guess we're already using env vars during testing anyway, so nothing gained over using env vars?). We could also re-check drush sql commands before we start implementing this, maybe there is now another solution that is even better?

@feyp I don't think there is anything to do with drush here. This is a core issue, and drush is not used to execute these commands.

I was not asking from the point of security (others will have an opinion on that). I just wanted to know how I can test this locally. Some other devs have reported being able to run this locally. Maybe the warning can be suppressed so that the tests continue?

@feyp I don't think there is anything to do with drush here. This is a core issue, and drush is not used to execute these commands.

Yes, I know that, of course. I was referring to the implementation of drush sql commands, which also call mysqldump, as a reference implementation on how to suppress this warning in code and was suggesting to do something similar here so that you don't run into this warning when running the tests locally.

That's because CI images using specific config and predefined users https://git.drupalcode.org/project/drupalci_environments/-/blob/dev/db/m...

I have this working locally now using the method @feyp suggested - writing the user and password to a temporary file then using --defaults-extra-file=$user_credentials_file instead of the -u$user -p$pass arguments. If this is an acceptable solution I will tidy it up and push it here for comments. But if this is a non-starter I will discard it.

Dumb question but good way to test this one?

I just kicked off a pipeline to check if it's still green and try to see what the timings look like.

Another way would be to run a test that is likely to benefit from the change locally before/after and record the times - although probably need to do middle of three to account for variations.

Seems to have some test failures

But left some comments/questions on the MR.

Changing to the DX special tag defined on Issue tags -- special tags → .

For Drupal 11 needs to remove

    \Drupal::configFactory()->getEditable('system.file')
      ->set('path.temporary', $this->tempFilesDirectory)
      ->save();

because path.temporary is deprecated