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.

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 #3416522: Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller which significantly improves performance of the installer.

MR actually appears to be red

I have rebased 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
11.x/core/tests/Drupal/TestSite/Commands/TestSiteInstallCommand.php?ref_type=heads#L27-31

There must be several ways to the solve the phpstan error here. Not sure which is preferred. It was attempted then reverted
https://git.drupalcode.org/project/drupal/-/merge_requests/5744/diffs?co...

I moved the definition of dumpFile from BrowserTestBase to FunctionalTestSetupTrait. Given that BrowsertestBase uses that trait this should still work, and it solves the PHPstan error.

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

Folks, I'm trying to add sqlite support. How to import sqlite dump?

exec("sqlite3 $db-$this->databasePrefix < $this->dumpFile");

doesn't work because NOCASE_UTF8 not registered (old drupal sqlite issue)