Improve performance of functional tests by caching Drupal installations

I really love this as an idea, we're running daily tests for the search ecosystem on travis and I'm constantly running into the travis time limit with facets: https://travis-ci.org/mkalkbrenner/search_api_integration_tests/builds/3...

1. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -257,6 +259,13 @@
   +   * @var
   +   */
   +  protected $dumpFile;
   

   @var string?

2. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -970,6 +979,44 @@ protected function getOptions($select, Element $container = NULL) {
   +
   +
   

   Should be one line.

3. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -970,6 +979,44 @@ protected function getOptions($select, Element $container = NULL) {
   +    sort($modules);
   

   Let's do, array_unique(sort($modules)); to make sure that when a class extends the another one and for some reason the same module is declared twice (because the base class changed or something like that).

4. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -970,6 +979,44 @@ protected function getOptions($select, Element $container = NULL) {
   +    $this->dumpFile = $cache_dir . '/_' . implode('-', $modules) . '.sql';
   

   Should we worry about filename length here? This can lead to really long filenames. Some of the contributed modules that extend already existing tests can have easily 8-9 modules.

5. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -981,6 +1028,94 @@ public function installDrupal() {
   +        exec("mysqldump -u$user -p$pass $db $tables_param | sed 's/$this->databasePrefix/default_db_prefix_/' > {$this->dumpFile}");
   

   I'm not sure this is something I like. Adding an exec isn't great, I also don't think we should add the sed here, that would make windows systems not able to use this, I think?

   I'm not sure that's the best solution.

6. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -981,6 +1028,94 @@ public function installDrupal() {
   +        exec("sed 's/default_db_prefix_/$this->databasePrefix/' $this->dumpFile | mysql -u$user -p$pass $db");
   

   Same here.

It seems to be that the experiment @fabianx did a while ago in #2759197: [D7] Improve WebTestCase performance by 50% did not required the mysqldump command. Have you seen this issue? Do you think we could incorporate that strategy?

@Chi
Your approach seems to for example not support pgsql, am I right?

In that case, I think we should close this issue and try to get #2747075: [meta] Improve WebTestCase / BrowserTestBase performance by 50% going again. At least a reroll on that issue should help. While it might not be as big an improvement as this issue, it still is an improvement and it will work for all databases.

I think we should close this issue

#2747075: [meta] Improve WebTestCase / BrowserTestBase performance by 50% is meta so I don't think we should close this one. This can be its child also the patch in #2747075 is for WTB and this is for BTB so this is more up to date.

I think this should just re-roll the latest patch from the meta issue - it was a functional port.

This issue is awesome!

I'm really liking this idea too. Functional tests are so useful, but it's such a pain how long they are taking.

Maybe instead of the exec approach, we could be doing something similar to how Drupal\FunctionalTests\Update\UpdatePathTestBase works by loading the DB from output generated from the dump script at scripts/dump-database-d8-mysql.php

Since it hasn't been mentioned here: Just a heads up that #2973227: Move UI helper functions from BTB into the traits was committed to support https://gitlab.com/weitzman/drupal-test-traits

That's a third-party testing framework that allows you do to functional tests against an existing site.

Created a reroll from scratch. Reverted DrupalKernel change. Addressed #8.1,2,3.

+++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
@@ -651,4 +651,94 @@ protected function getDatabaseTypes() {
+      case 'mysql':
+        $tables = \Drupal::database()
+          ->query("SHOW TABLES LIKE '$this->databasePrefix%'")
+          ->fetchCol();
+        $tables_param = implode(' ', $tables);
+        exec("mysqldump -u$user -p$pass $db $tables_param | sed 's/$this->databasePrefix/default_db_prefix_/' > {$this->dumpFile}");
+        break;
+
+      default:
+        throw new \LogicException('This database driver is not supported yet.');
...
+    switch ($connection_info['default']['driver']) {
+      case 'mysql':
+        exec("sed 's/default_db_prefix_/$this->databasePrefix/' $this->dumpFile | mysql -u$user -p$pass $db");
+        break;
+
+      default:
+        throw new \LogicException('This database driver is not supported yet.');

This might be an over-engineering suggestion, but if this were instead implemented as a plugin, then in theory non-core DB implementations could also utilize this, and the code would be a bit cleaner as all db-specific loading/dumping logic would be encapsulated in the plugins...

I agree the patch is little rough around the edges but before proceeding with this we should have some kind of consensus from contributor or approval from maintainers.

Would really love this patch! However, I hit the following error. The test has quite a lot of dependencies and I'm running in DDev, not sure if the either (or both) of those are triggering the issue:

PHPUnit\Framework\Exception: ERROR 1146 (42S02) at line 190: Table 'db.test58600546bookkeeping_transaction__related' doesn't exist
sed: couldn't write 589982 items to stdout: Broken pipe

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -565,6 +572,44 @@ protected function getOptions($select, Element $container = NULL) {
+    while ($class) {
+      if (property_exists($class, 'modules')) {
+        $modules = array_merge($modules, $class::$modules);
+      }
+      $class = get_parent_class($class);
+    }
+    sort($modules);
+    array_unique($modules);
...
+    $this->dumpFile = $cache_dir . '/_' . implode('-', $modules) . '.sql';

For something in contrib this would be a huge filename and probably crash some filesystems, maybe Windows which has problems with large file paths.

Perhaps a hash of the modules array is more appropriate.

The topic of making testing significantly faster (and thus more affordable) has certainly become top of mind again.

I would love to help bring attention to this effort using DA channels, if that would be helpful.

It's also rather late in the game to be suggesting alternative approaches, but another suggestion from the world of containerization could be to scaffold testing using a base, preinstalled Drupal environment, build new instances on top of that base for each test, resetting to the base each time to ensure a clean environment.

The patch of #22 cut down my testing time in half (35 to 16 minutes)!

Although it's not the cleanest solution as mentioned before, i've made some changes:

- The dumpFile's filename is now a md5 hash of the imploded module list
- Tests using the public and private filesystems failed
- mysqldump / mysql now takes the database hostname in account.

Allright.. The correct patch file here.

Fixed coding standards error.

Patch in #33 worked for me. Brought test time down from 12.7 minutes to 56 seconds in my case.

1. +++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
   @@ -711,4 +711,113 @@ protected function getDatabaseTypes() {
   +    $settings['settings']['hash_salt'] = (object) [
   +      'value' => $this->databasePrefix,
   +      'required' => TRUE,
   +    ];
   +
   +    $settings['settings']['hash_salt'] = (object) [
   +      'value' => $this->databasePrefix,
   +      'required' => TRUE,
   +    ];
   

   Hash salt is duplicated and could be set to Crypt::randomBytesBase64(55) which i think is more correct.

2. +++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
   @@ -711,4 +711,113 @@ protected function getDatabaseTypes() {
   +    $connection_info = Database::getConnectionInfo('default');
   +
   +    $user = $connection_info['default']['username'];
   +    $pass = $connection_info['default']['password'];
   +    $db = $connection_info['default']['database'];
   +    $host = $connection_info['default']['host'];
   +
   +    switch ($connection_info['default']['driver']) {
   +      case 'mysql':
   +        $tables = \Drupal::database()
   +          ->query("SHOW TABLES LIKE '$this->databasePrefix%'")
   +          ->fetchCol();
   +        $tables_param = implode(' ', $tables);
   +        exec("mysqldump -u$user -p$pass -h $host $db $tables_param | sed 's/$this->databasePrefix/default_db_prefix_/' > {$this->dumpFile}");
   +        break;
   +
   +      default:
   +        throw new \LogicException('This database driver is not supported yet.');
   +    }
   

   Instead of doing this we can use the database dump command.

           // Create DB dump.
           $command = new DbDumpCommand();
           $command_tester = new CommandTester($command);
           $command_tester->execute([]);
           file_put_contents($path_to_db, $command_tester->getDisplay());
   

   Then to install from a dump we can do:

         require $path_to_db;
   

3. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -568,6 +575,44 @@ protected function getOptions($select, Element $container = NULL) {
   +    $this->dumpFile = $cache_dir . '/_' . md5(implode('-', $modules)) . '.sql';
   

   This also needs to include the default theme and profile.

4. I have some custom code on a client that does something very similar. However is only overrides the doInstall() method - so it has less chance of going wrong because it doesn't have to care about all the calls in installDrupal(). And it is only about installing the profile so it makes this more useful to real projects and more tests are sped up because installing a single extra module is quite quick. See https://gist.github.com/alexpott/8cd82bc75b88c01489d837cf8432451b

Also I don't think this is useful for 99% of core browser tests - we use the testing profile and only ever install a few modules. It won't make as much difference now the install is much quicker. But this would be super super useful functionality for real projects with their own dump or install profile.

We do need test coverage to prove things actually work.

@Chi that's good for running single core tests but if you testing a site build with a large install profile (lots of modules and config) using a dump really really helps. Also I think in-memory sqlite dbs suffer when you do testing via webdriver (or anything that uses a real browser) because of the multiple requests. Like even create image style derivatives can cause a sqlite memory db to be really slow compared to another db type.

What level of speed improvment is being aimed for here? Is everyone testing against a common test here?
Let's establish a baseline:
Note: DemoTest.php is the simpliest / fastest functional test I could think of. I understand that functional test usually involve enabling modules, creating nodes, manipulating users. With a common baseline, it's easier to establish setups, settings, pain-points, that can be targeted or improved.

Setup:
- Copy DemoTest.php_.test into <drupal_root>/modules/custom/demo/tests/src/Functional
- Rename DemoTest.php_.test -> DemoTest.php (I can't upload a PHP file to this site)
- Run PHPunit, tartgeting test.

phpunit .\web\modules\custom\demo\tests\src\Functional\DemoTest.php

Clean drupal install results:

Time: 00:39.273, Memory: 10.00 MB
Time: 00:36.829, Memory: 10.00 MB
Time: 00:35.932, Memory: 10.00 MB

- Apply 2900208-33.patch
- Run PHPunit, tartgeting test.

phpunit .\web\modules\custom\demo\tests\src\Functional\DemoTest.php

Clean drupal install results:

Time: 00:37.624, Memory: 10.00 MB
Time: 00:36.372, Memory: 10.00 MB
Time: 00:37.048, Memory: 10.00 MB

- No noticable difference but then the test isn't really doing anything other then starting up.

@Chi the Drupal installer has had significant performance improvements since then so any benchmark from 2017 needs to be re-run. As I've pointed out the real utility for this is on real sites where installing your real site can take quite sometime because you're dealing with way more modules and config and sometimes content. I think we should provide a test trait along the lines of https://gist.github.com/alexpott/8cd82bc75b88c01489d837cf8432451b that we document or even consider automatically using when the profile is not one of the core testing profiles.

Perhaps the Umami test would be a better common baseline:
https://api.drupal.org/api/drupal/core%21profiles%21demo_umami%21tests%2...

@joevagyok has been using this patch at the European Commission for years, and it improves test performance ~4x!

I think this issue may have been underestimating the indirect benefits for concrete projects. Even if it's not 4x but just 1.5x faster … then it would be a huge productivity boost for many Drupal projects.

@joevagyok will push this forward tomorrow :)

We are testing out this patch, to help with our 26min+ functional test runs, and for the most part it seems to be working as expected. However, one of our tests checks the X-Drupal-Cache-Tags header and we noticed that on initial runs (no DB dump yet) the tests pass, but on subsequent runs (restoring from the DB dumb) the tests fail, and that's because the X-Drupal-Cache-Tags header is not present.

This seems to be due to the fact that FinishResponseSubscriber::debugCacheabilityHeaders is false.

In tests which run installDrupalFromProfile() the cacheability headers are set by calling $this->setContainerParameter('http.response.debug_cacheability_headers', TRUE); from within initSettings(). That never gets called when tests run installDrupalFromDump()

This patch addresses that problem, with an interdiff provided too.

Re-rolled patch #47 to apply to 9.4.6

Hey, we had a similar case but limited in scope. Our project has only kernel tests and uses sqlite, so I wrote a module that handles that. Feel free to test it out / benchmark in your projects if that fits your case: https://www.drupal.org/project/sqlite_test_cache

Is this issue still in "Needs work" or "Needs review"? I see a new patch was added at #48, just not sure if it addresses all of #35 feedback.

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". Needs to remove old dump

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.

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 #3406929: Configuration being imported by the ConfigImporter sometimes has stale original data (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.