Allow BTB to test an existing, already installed Drupal site instead of installing from scratch

Note: github.com/dawehner/sitetestbase and https://www.drupal.org/node/2793443

IMHO ideally we would have a flag to make this possible. A flag which is controlled by the test class itself. In the case this flag is set, it would setup the right database credentials by loading it from settings.php and call it a day.

Adapted from our base class

No idea how to test this.

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -1754,4 +1828,98 @@ protected function getConfigSchemaExclusions() {
+  /**
+   * When running against an installed site, mark an entity for deletion.
+   *
+   * Any entities you create when running against an installed site should be
+   * flagged for deletion to ensure isolation between tests.
+   *
+   * @param \Drupal\Core\Entity\EntityInterface $entity
+   *   Entity to delete.
+   */
+  protected function markEntityForCleanup(EntityInterface $entity) {
+    $this->cleanupEntities[] = $entity;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function drupalPlaceBlock($plugin_id, array $settings = array()) {
+    $block = $this->drupalPlaceBlockParent($plugin_id, $settings);
+    if ($this->runAgainstInstalledSite) {
+      $this->markEntityForCleanup($block);
+    }
+    return $block;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function drupalCreateContentType(array $values = array()) {
+    $type = $this->drupalCreateContentTypeParent($values);
+    if ($this->runAgainstInstalledSite) {
+      $this->markEntityForCleanup($type);
+    }
+    return $type;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function drupalCreateNode(array $settings = array()) {
+    $node = $this->drupalCreateNodeParent($settings);
+    if ($this->runAgainstInstalledSite) {
+      $this->markEntityForCleanup($node);
+    }
+    return $node;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function drupalCreateUser(array $permissions = array(), $name = NULL, $admin = FALSE) {
+    $user = $this->drupalCreateUserParent($permissions, $name, $admin);
+    if ($this->runAgainstInstalledSite) {
+      $this->markEntityForCleanup($user);
+    }
+    return $user;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function drupalCreateRole(array $permissions, $rid = NULL, $name = NULL, $weight = NULL) {
+    $role = $this->drupalCreateRoleParent($permissions, $rid, $name, $weight);
+    if ($this->runAgainstInstalledSite) {
+      $this->markEntityForCleanup($role);
+    }
+    return $role;
+  }

Wouldn't it be nice if we would have some entity subscriber which tracks all created entities automatically?

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -549,9 +583,19 @@ protected function cleanupEnvironment() {
+    if ($this->runAgainstInstalledSite) {
...
+      foreach ($this->cleanupEntities as $entity) {
+        if ($entity instanceof EntityInterface) {
+          $entity->delete();
+        }

@@ -1754,4 +1828,98 @@ protected function getConfigSchemaExclusions() {
+    if ($this->runAgainstInstalledSite) {
+      $this->markEntityForCleanup($node);
+    }
...
+    if ($this->runAgainstInstalledSite) {
+      $this->markEntityForCleanup($user);
+    }
...
+    if ($this->runAgainstInstalledSite) {
+      $this->markEntityForCleanup($role);
+    }

We do the check in cleanupEnvironment() anyway, can't we just unconditionally add the entities into $cleanUpEntities?

This was all I needed to actually make this work, didn't look at any of the other comments.

IMHO this totally falls into it

13.37 KB

Nice patch size.

Can we can an in

I would totally like that as a feature for sites testing. I know at least frankcarey would be also highly interested in the feature in general.

Regarding approval or not, I think it would help to provide some concrete example in the issue summary.
On top of that it would be nice to maybe open up a follow up to deal with the problem of automatic cleanup (somehow listen to all created entities and revert them).

@Sam152
To be honest I have no clue who would have to approve this feature.

Bit of clean-up here, the only way I can see us automatically picking up the created entities would be via hook_entity_update/insert but that would require a module enabled in every test. Once #2551893: Add events for matching entity hooks is in we could register an event listener in the test setup much easier.

We could also add the already installed check in markEntityForCleanup if memory usage is a concern in bigger tests.

I have a different approach to this problem which I found to be fast and effective at rooting out bugs: I use KernelTestBase and a test module. The test module has a config/install directory which contains symlinks to files in the config sync directory of the live site. This allows to recreate a fraction of the live site precisely and quickly. The test are runs very quick since it doesn't need to fully bootstrap Drupal even once. It also allows me to mock anything that needs mocking.

This is functional testing. I know it's tangential to the BTB -- but unless you are JS testing, this is functional testing. A bit crazy but real, just don't forget to enable all the modules.

Yes, per Daniel, I'm very interested in this especially where we can the same tools for testing between core and individual sites. (and hopefully across Behat / phpsec/ BTB / etc)

One thing we're doing in the (new) TestDrupal behat extension is keeping track on any entities that are created so that we can:

• Make it easy to reference entities using their titles in behat steps since you won't know their IDs ahead of time
• Clean up entities between scenarios automatically

The problem is that we're only keeping track of the entities that are created in behat steps.. we don't actually know if any other entities are created. I'm at BADCamp right now and I'm working on a new module called testdrupal_helper that among other things will map entity creation to new symphony events as was mentioned above. I'll let you know how that comes along and will post some code for some feedback.

The problem is that we're only keeping track of the entities that are created in behat steps.. we don't actually know if any other entities are created. I'm at BADCamp right now and I'm working on a new module called testdrupal_helper that among other things will map entity creation to new symphony events as was mentioned above. I'll let you know how that comes along and will post some code for some feedback.

That's not something you should do or even care about in Behat test scenarios. Behat is about BDD, it is intended to describe user behaviour. If the website creates any entities behind the scenes which the user doesn't know about then your Behat scenario should not deal with this. In short, if you are using Behat to write functional tests instead of describing user behaviour scenarios you are not using it correctly. You should use BTB for functional tests.

Where this will all fall apart is when you have 2 tests running simultaneously against the same installation. BTB will be able to handle this fine. Testdrupal_helper won't. The best way to solve this in practice is to use unique labels for your entities in every Behat scenario and add some steps at the end to find the entities that were created manually the test by label and clean them up.

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -1767,4 +1852,108 @@ protected function getConfigSchemaExclusions() {
+  /**
+   * {@inheritdoc}
+   */
+  protected function drupalCreateUser(array $permissions = array(), $name = NULL, $admin = FALSE) {
+    $user = $this->drupalCreateUserParent($permissions, $name, $admin);
+    $this->markEntityForCleanup($user);
+    return $user;
+  }

I'm not sure if this pattern of overriding all traits that create entities is going to be practical, especially for contrib and custom tests. For core tests it will probably be fine, but what happens if a contrib module defines a bunch of traits too for creating their entities? We won't be able to override them in BTB, so this means that BTB should be subclassed. This will get messy fast when you are writing a test for something that involves multiple contrib modules.

Maybe we should add support for entity cleanup in the traits themselves. Traits are intended to be dropped in to tests when needed, and ideally without requiring any local overrides.

Inside the traits we can detect whether we are running a test that uses a persisting database, and call $this->markEntityForCleanup() in it.

I think the cleanest solution would be to make BrowserTestBase implement a new PersistentTestDatabaseInterface which declares the markEntityForCleanup() method so we can use this to detect support for entity cleanup in the traits:

  protected function drupalCreateUser(array $permissions = array(), $name = NULL, $admin = FALSE) {
    // ... Create the user.
    
    if ($this instanceof PersistentTestDatabaseInterface) {
      $this->markEntityForCleanup($user);
    }

    return $user;
  }

#2796105: Move similar methods in BrowserTestBase / WebTestBase to a trait; untangle installDrupal() is in. NW as per #25.

It's also worth noting you don't always have an instance of the entity that has been created. Submitting a form doesn't return the entity for example.

If any entity is created through the UI then it will need to be cleaned up manually at the end of the test. I don't think there is a reliable way to automate this.

@pfrenssen , I think you are mostly right in saying ".. if you are using Behat to write functional tests instead of describing user behaviour scenarios you are not using it correctly." However, say you have an entity that when created, creates other entities.. (commerce comes to mind). When you need to do your cleanup after a scenario, it's very hard to track all of these things that need to be deleted if you don't have some way to hook in. Your suggestion to use a specific "label" may work in some cases, but not all. The alternative is doing full reinstall or to revert the database to a previous dump which often takes longer than necessary. Also the @Given steps, which prepare the situation to be tested, certainly don't have to be done using user behavior and often that isn't an efficient way to go anyways.

You said, "Where this will all fall apart is when you have 2 tests running simultaneously against the same installation.", but Behat doesn't allow for simultaneous tests anyways. The way we run tests in parallel is to have completely separate environments and run a subset of the features/scenarios in each. That said, there may be a way to make this more feasible.. perhaps a header in the request or in the session that sets which environment we're dealing with?

I agree with @Sam152 "I think dawehner's suggestion of having an event subscriber or entity lifecycle hook to track these would be viable." I don't think it's necessary to pollute things with markup like '$this->markEntityForCleanup($user);'.. I haven't made any progress with that module since BADCamp, but I'll post here if I start working on it again.

Here's a re-roll against 8.4

Here's a first attempt at using an event listener to clean-up in the tests, depends on #2551893: Add events for matching entity hooks and the current EntityCleanup tests needs to be run against an existing installed site, I was using Standard locally. Not sure if we can make it run on the bot yet.

Also the original patch re-rolled against 8.4.x with a fix from a a mistake i made in the re-roll.

Original patch re-rolled for 8.3.0, anyone want to review?

My remark from #30 is not yet addressed.

Moving the checks to the traits makes sense if that was our end goal but I still think a better solution is what I proposed in #39 because that will pick up all entities that were created in a generic way. However, it's dependant on #2551893: Add events for matching entity hooks and who knows how long that issue will take.

The problem with moving the methods to the traits is, we have to then support clean-up in the future for people using those traits, which isn't so easy if we move to an event listener which would be registered once for all entities, not based on the traits you were using.

OK I agree, I proposed to use the separate interface and the traits as a way to fix the current state of the patch, but if we can make it work automatically then it's even better, so that's definitely worth exploring.

I have been thinking about the event listener and we can probably identify entities created during the test by associating it with the test prefix from drupal_valid_test_ua(). We cannot use semaphores or a similar time based approach because it would also catch any other entities created outside of the test while it is running.

Fixed the remaining failures.

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -1369,4 +1456,88 @@ protected function getTestMethodCaller() {
+    $role = $this->drupalCreateRoleParent($permissions, $rid, $name, $weight);
+    $this->markEntityForCleanup(Role::load($role));
+    return $role;

Thats the only change i made, drupalCreateRoleParent returns only the role id. Not the entity.

Re-rolled the patch against 8.4.x branch because it's failed to apply.

With the current patch the modules defined under $modules will not be installed. Should we forbid having listed any modules if running against the current installed Drupal and throw an exception in such a case?

Just discovering this issue...

I think it's a bad idea to test without a fixture. It's like doing dev on production. But it might also be good to have a more formalized Drupal alternative to Behat or other systems. Generally -1 from me.

But since it's +1 from everyone else: Make it a new base class that extends from BTB. That way you're not always making the test framework more complex with a lot of if ($this->runAgainstInstalledSite) special cases, and we're not promising that BTB will work this way forever. Plus it's much easier to document.

Also:

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
@@ -23,7 +23,7 @@
+    $path = DRUPAL_ROOT . '/' . $this->tempFilesDirectory . DIRECTORY_SEPARATOR . 'browsertestbase-templatecache';

Use $this->container->get('app.root') because DRUPAL_ROOT is pure evil.

\Drupal\simpletest\UserCreationTrait::createAdminRole() creates \Drupal\simpletest\UserCreationTrait::createRole() directly so we have to add

    $rid = reset($user->getRoles(TRUE));
    $this->markEntityForCleanup(Role::load($rid));

to the \Drupal\Tests\BrowserTestBase::drupalCreateUser().

Yet another reroll also address #55.

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -1415,6 +1415,9 @@ protected function drupalCreateNode(array $settings = array()) {
   protected function drupalCreateUser(array $permissions = array(), $name = NULL, $admin = FALSE) {
...
+    foreach ($user->getRoles(TRUE) as $rid) {
+      $this->markEntityForCleanup(Role::load($rid));

What if the role actually already exists on the currently installed instance? In this case this will delete an existing role, not created in the test.

What if the role actually already exists on the currently installed instance? In this case this will delete an existing role, not created in the test.

That is not true. There is no way to pass the current role to \Drupal\Tests\user\Traits\UserCreationTrait::createUser. Instead, you can do this

    $user = $this->drupalCreateUser();
    $user->addRole('some_role_on_site');
    $user->save();

When executing this method yes, but one might override the method drupalCreateUserParent() and add existing roles to the user entity instead of creating new ones. An other option is that some of the functionality of UserCreationTrait is being overwritten. You never know what a developer might implement, therefore we should not depend on the implementation underneath, but mark an entity for deletion only if we are sure that we've created it during a test, and the best place for this is where we call $storage->create().

Has it been considered to mark entities for deletion in hook_entity_create() during tests instead? This will cover each entity, which is not currently done by the patch, and reduce the complexity as well.

Oh, I just saw that @frankcarey has been talking on something like this.

When executing this method yes, but one might override the method drupalCreateUserParent() and add existing roles to the user entity instead of creating new ones. An other option is that some of the functionality of UserCreationTrait is being overwritten. You never know what a developer might implement, therefore we should not depend on the implementation underneath, but mark an entity for deletion only if we are sure that we've created it during a test, and the best place for this is where we call $storage->create().

Well, this is true for all the methods using markEntityForCleanup.

This is a stop gap approach till #2551893: Add events for matching entity hooks is in.

Has it been considered to mark entities for deletion in hook_entity_create() during tests instead? This will cover each entity, which is not currently done by the patch, and reduce the complexity as well.

See the patch in #39

Re #54, the only issue with that is support for both BTB and JTB. We can't easily compose test classes with different drivers, so we're sorta stuck building it into the test base.

OK, so then turn JTB into a trait. It's small.

Make this class heirarchy: BTB -> PreExistingBTB -> PreExistingJTB use JTBTrait.

Also we should add a test listener that fails a test run if core tests use these pre-existing fixtures.

Reroll for 8.4.0

I think before moving forward we need to ask the subsystem maintainer and framework manager about their opinion.

I'd much rather see a way to make a repeatable fixture site based on config and some entities in a dehydrated form, rather than an open-ended extension to BTB that lets you un-isolate a functional test by design.

Having a more behavioral-oriented framework (which is what this is) separate from BTB would also be useful. That's why I raised concerns in #54 (and #65).

I think we should get back to forward porting the improvements done to simpletest in D7 first.

#2796105: Move similar methods in BrowserTestBase / WebTestBase to a trait; untangle installDrupal() was the first step for that.

#2747075: [meta] Improve WebTestCase / BrowserTestBase performance by 50% is the meta.

#2795749: [Testing issue] Make tests faster was the last patch.

When those are in, this might be moot - and we'll retain the isolation

I'd much rather see a way to make a repeatable fixture site based on config and some entities in a dehydrated form

You mean #2788777: Allow a site-specific profile to be installed from existing config?

I think we should get back to forward porting the improvements done to simpletest in D7 first.

Unfortunately, #2747075: [meta] Improve WebTestCase / BrowserTestBase performance by 50% has been stalled for more than a year now :(

When those are in, this might be moot

I'm confused, how can performance improvements help us allow running BTB test to an existing site?

I think all @larowlan is saying: We might not really need the functionality if our tests become blazing fast.

Thanks @dawehner, that's what I meant :)

I think something like this would still be really great to replace Behat for sites (not core testing). As things stand now, custom code can be tested for sites using our phpunit-based testing framework. This breaks down though when the tests need to rely on a site's entire configuration (eg, display modes, access, etc), rather than a small subset of modules.

Currently, Behat fills this gap, but if used in a non-BDD manner, it's not really being used correctly. It still continues to be used though because tests can be run against a completely installed site.

That's not to say this need be in core--it could be a contrib module trait or something that allows tests to swap the test environment...

Updated patch #66 for 8.4.4

Yet another reroll

Any thoughts re: #76 as a response to the above concerns?

With these changes in core, a contrib project could pick up from here and allow for re-usable entity cleanup by extending common core traits such as NodeCreationTrait and tracking created nodes as the Drupal Behat Extension does now.

+++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
@@ -595,69 +595,104 @@ protected function prepareEnvironment() {
+    if (!$this->runAgainstInstalledSite) {

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -47,27 +49,58 @@
+  protected $runAgainstInstalledSite = FALSE;

Please turn this into a trait that doesn't require any changes to BTB and is not used by BTB.

That way when you want this behavior you subclass BTB and then use the trait, and no one has to go hunting for a variable being set somewhere.

Minimizing the needed changes to BTB and JTB would be nice, especially since core itself will never use any of this. We just need to make the changes necessary so an external library is able to utilize these classes to test existing sites I think.

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -47,27 +49,58 @@
-    placeBlock as drupalPlaceBlock;
+    placeBlock as drupalPlaceBlockParent;
...
-    createNode as drupalCreateNode;
...
-    createContentType as drupalCreateContentType;
+    createContentType as drupalCreateContentTypeParent;
...
-    createRole as drupalCreateRole;
-    createUser as drupalCreateUser;
+    createRole as drupalCreateRoleParent;
+    createUser as drupalCreateUserParent;
...
+  /**
+   * Entities to clean up.
+   *
+   * @var \Drupal\Core\Entity\EntityInterface[]
+   */
+  protected $cleanupEntities = [];
...
+  /**
+   * Config to reset.
+   *
+   * @var array
+   */
+  protected $resetConfig = [];
...
+  protected function drupalCreateUser(array $permissions = array(), $name = NULL, $admin = FALSE) {
+    $user = $this->drupalCreateUserParent($permissions, $name, $admin);
...
+  protected function drupalCreateRole(array $permissions, $rid = NULL, $name = NULL, $weight = NULL) {
+    $rid = $this->drupalCreateRoleParent($permissions, $rid, $name, $weight);

Since core will never user any of this, I think they could be moved to an external library as a handful of traits that use the core traits, something like:

trait NodeCreationTrait {

  use CoreNodeCreationTrait {
    createNode as coreCreateNode;
  }

  /**
   * Create a node and track it for cleanup.
   *
   * @param array $settings
   */
  public function createNode(array $settings = []) {
    $node = $this->coreCreateNode($settings);
    $this->cleanupEntities[] = $node;
  }

The tracking of cleanup entities (and the removal) could be another trait in that external library.

Minimizing the needed changes to BTB and JTB would be nice, especially since core itself will never use any of this.

+1

We need to re-roll 2793445-39-do-not-test.patch from #39 which depends on #2551893: Add events for matching entity hooks. Once that's done everything in #82 will be addressed.

Please turn this into a trait that doesn't require any changes to BTB and is not used by BTB.

I think this should be a separate base class whether it should live in core or contrib it is another discussion,

Minimizing the needed changes to BTB and JTB would be nice, especially since core itself will never use any of this.

I disagree with this assumption. We have an experimental profile in core now with default content and blocks. I think the testing of those things will hugely benefit from this feature.

Please turn this into a trait that doesn't require any changes to BTB and is not used by BTB.

If we create a new trait and inherit a new base class from BTB and use the new trait in it then PHP 5 will not understand which ::prepareEnvironment should be used, the one from \Drupal\Core\Test\FunctionalTestSetupTrait or the one from new trait. Therefore, I suggested above that this should be a separate base class.

Reroll after #2775653: JavascriptTests with webDriver

This patch adds an mkdir for the writing of the htkey file. This is the same as #2246725: Make sure TestSitePath exist before creating .htkey but since that issue is closed, we include it here.

Ugh. Again

triggerin' the testin'

Still doesn't address #81.

1.   /**
      * Prepares the current environment for running the test.
      *
      * Also sets up new resources for the testing environment, such as the public
      * filesystem and configuration directories.
      *
      * This method is private as it must only be called once by
      * BrowserTestBase::setUp() (multiple invocations for the same test would have
      * unpredictable consequences) and it must not be callable or overridable by
      * test classes.
      */
     protected function prepareEnvironment() {
   

   Docblock is now incorrect.

2. +++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
   @@ -588,69 +588,104 @@ protected function prepareEnvironment() {
   +    if (!$this->runAgainstInstalledSite) {
   ...
   +    else {
   

   prepareEnvironment() is now 103 lines of code, which is one of the reasons this should be a separate base class or a separate trait.

   Please turn the else section into another method called something like preparePreExistingEnvironment() instead of adding complexity, and flip the logic on runAgainstInstalledSite.

Just discovered https://github.com/weitzman/drupal-test-traits/. Nice one @moshe weitzman. I think we can 'won't fix' this as soon as https://github.com/weitzman/drupal-test-traits/issues/2 and https://github.com/weitzman/drupal-test-traits/issues/3 are addressed.

Also, the traits in that project don't handle automatically marking created users, node, blocks etc for clean up?

We could also add @see to BTB here, pointing to that repo.

I hope folks use and contribute to https://github.com/weitzman/drupal-test-traits/. I think its a solid alternative to this issue. My intent is to hit a sweet spot where the project is useful and still lean.

I agree that this issue can be closed.

For people like me who are still using this patch with 8.6

For people like me who are still using this patch with 8.7

Thanks @larowlan!

Reroll for those still using the patch on 8.9.

Moshe did not get a lot of traction and archived the repo. back in 2018. More up to date activity here. Re-opening.

The project moved to Gitlab -> https://gitlab.com/weitzman/drupal-test-traits

Can vouch the project works and is well maintained.

Ah, thank you :)