Move similar methods in BrowserTestBase / WebTestBase to a trait; untangle installDrupal()

let's see what blows up with this first attempt

whoops - got the inheritance slightly wrong

gah, muppet

still changing the inheritance

passRaw pass_raw mismatch

Here is a conceptual question. If you look into TestSetupTrait and BrowserTestBase you see BTB overriding prepareDatabasePrefix.
This feels weird, because simpletest is the one that does actually more. Maybe we should move the override to WTB?

1. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -362,7 +273,7 @@ protected function initMink() {
       *
   -   * @return Behat\Mink\Driver\DriverInterface
   +   * @return \Behat\Mink\Driver\DriverInterface
       *   Instance of default Mink driver.
       *
   

   Nitpick: out of scope change :P

2. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -977,161 +888,14 @@ protected function getOptions($select, Element $container = NULL) {
   +    $this->initUserSession();
   +    $this->prepareSettings();
   +    $this->doInstall();
   +    $this->initSettings();
   +    $container = $this->initKernel(\Drupal::request());
   +    $this->initConfig($container);
   +    $this->installModulesFromClassProperty($container);
   +    $this->rebuildAll();
      }
   

   This is really nice code, IMHO

addressing feedback

killing time √

Super nice work! This trait could be of course improved, but that would be out of the scope of this issue.

I have rerolled the patch for 8.3.x branch.

@Fabianx - I have rerolled the patch to work with 8.3.x version.
please review

Just a reroll as part of the research of moving UpdatePathTestBase

Fix attached.

Edit: not actually sure why this works. I guess $this->$key = $value in UserSession::__construct isn't enough?

Ah, figured it out. So, passRaw is required by btb and pass_raw is required by wtb. This is also referenced in UserCreationTrait:

    // Add the raw password so that we can log in as this user.
    $account->pass_raw = $edit['pass'];
    // Support BrowserTestBase as well.
    $account->passRaw = $account->pass_raw;

oh right, we've been there already biting us. Its good to have that so we have a better BC layer.

Thank you @Sam152 for the reroll!

Rerolled

I assume it was a random failure. Back to RTBC.

1. It's nice to see this code being shared between Simpletest and BrowserTestBase - certainly will make maintaining both through the D8 lifecycle simpler :)

2. +++ b/core/lib/Drupal/Core/Test/TestSetupTrait.php
   @@ -0,0 +1,196 @@
   +  protected function prepareDatabasePrefix() {
   ...
   +  protected function changeDatabasePrefix() {
   
   +++ b/core/modules/simpletest/src/TestBase.php
   @@ -1125,36 +1023,6 @@ private function prepareDatabasePrefix() {
   -  private function changeDatabasePrefix() {
   
   +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -1241,65 +1005,6 @@ protected function installParameters() {
   -  private function prepareDatabasePrefix() {
   ...
   -  private function changeDatabasePrefix() {
   

   Change of scope - is that really right? Can a trait add a private method - I think it can. I'm not sure though.

3. +++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
   @@ -0,0 +1,447 @@
   +trait FunctionalTestSetupTrait {
   ...
   +    $this->container = $this->kernel->rebuildContainer();
   
   +++ b/core/lib/Drupal/Core/Test/TestSetupTrait.php
   @@ -0,0 +1,196 @@
   +  /**
   +   * The dependency injection container used in the test.
   +   *
   +   * @var \Symfony\Component\DependencyInjection\ContainerInterface
   +   */
   +  protected $container;
   

   I think FunctionalTestSetupTrait should use TestSetupTrait so things like container are declared and I think you'd always want TestSetupTrait if you're using FunctionalTestSetupTrait.

4. +++ b/core/lib/Drupal/Core/Test/TestSetupTrait.php
   @@ -0,0 +1,196 @@
   +  protected $kernel;
   +  /**
   

   Missing new line.

first a reroll

1) agree
2) done
3) if I do that, I get Drupal\simpletest\TestBase and Drupal\Core\Test\FunctionalTestSetupTrait define the same property ($configSchemaCheckerExclusions) in the composition of Drupal\simpletest\WebTestBase. However, the definition differs and is considered incompatible. i.e. TestBase in simpletest already includes the TestSetupTrait. So, in summary WebTestBase gets TestSetupTrait via inheriting it from TestBase. BrowserTestBase gets it from using both TestSetupTrait and FunctionalTestSetupTrait. Unfortunately, we're stuck with the deprecated KernelTestBase until 9.x, so until then, we can't get rid of TestBase and make TestSetupTrait used by FunctionalTestSetupTrait.
4) done

paraphrasing #48.3

test test test blah test blah blah test test test blah i heard you like test with your test blah blah test test

This certainly belongs into the phpunit initiative

Thanks @dawehner

1. Sorry for taking so long to re-review it
2. Removing the avoid commit conflicts tag - not sure really what the tag means and its purpose here

3. +++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
   @@ -0,0 +1,448 @@
   +    // @see TestBase::prepareEnvironment()
   ...
   +   * @see TestBase::prepareEnvironment()
   +   * @see TestBase::restoreEnvironment()
   ...
   +    // TestBase::restoreEnvironment() will delete the entire site directory.
   
   +++ b/core/lib/Drupal/Core/Test/TestSetupTrait.php
   @@ -0,0 +1,198 @@
   +use Drupal\simpletest\TestBase;
   ...
   +   * This is set in TestBase::prepareEnvironment().
   ...
   +   * This is set in TestBase::prepareEnvironment().
   ...
   +   * This is set in TestBase::prepareEnvironment().
   ...
   +   * @see TestBase::prepareEnvironment()
   

   Unused apart from in docs. And these docs need improving since they all point to TestBase but they need to point to both Drupal\simpletest\TestBase and \Drupal\Tests\BrowserTestBase

4. +++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
   @@ -0,0 +1,448 @@
   +  /**
   +   * Resets all data structures after having enabled new modules.
   +   *
   +   * This method is called by \Drupal\simpletest\WebTestBase::setUp() after
   +   * enabling the requested modules. It must be called again when additional
   +   * modules are enabled later.
   +   */
   +  protected function resetAll() {
   

   So is this method only used by WebTestBase? I think the main use is \Drupal\Core\Test\FunctionalTestSetupTrait::rebuildAll() which is called by both WebTestBase::setUp() and Drupal\Tests\BrowserTestBase::installDrupal()

5. FILE: ...s/devdisk/dev/drupal/core/modules/simpletest/src/WebTestBase.php
   ----------------------------------------------------------------------
   FOUND 0 ERRORS AND 10 WARNINGS AFFECTING 10 LINES
   ----------------------------------------------------------------------
     6 | WARNING | [x] Unused use statement
    11 | WARNING | [x] Unused use statement
    12 | WARNING | [x] Unused use statement
    15 | WARNING | [x] Unused use statement
    19 | WARNING | [x] Unused use statement
    21 | WARNING | [x] Unused use statement
    24 | WARNING | [x] Unused use statement
    25 | WARNING | [x] Unused use statement
    32 | WARNING | [x] Unused use statement
    34 | WARNING | [x] Unused use statement
   ----------------------------------------------------------------------
   PHPCBF CAN FIX THE 10 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   ----------------------------------------------------------------------
   
   Time: 186ms; Memory: 18Mb
   
   
   FILE: ...s/devdisk/dev/drupal/core/tests/Drupal/Tests/BrowserTestBase.php
   ----------------------------------------------------------------------
   FOUND 0 ERRORS AND 7 WARNINGS AFFECTING 7 LINES
   ----------------------------------------------------------------------
    10 | WARNING | [x] Unused use statement
    15 | WARNING | [x] Unused use statement
    16 | WARNING | [x] Unused use statement
    18 | WARNING | [x] Unused use statement
    19 | WARNING | [x] Unused use statement
    22 | WARNING | [x] Unused use statement
    30 | WARNING | [x] Unused use statement
   ----------------------------------------------------------------------
   PHPCBF CAN FIX THE 7 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   ----------------------------------------------------------------------
   

   Lovely let's get rid of all these uses then!

Never seen that fail in Drupal\filter\Tests\FilterFormatAccessTest before - retesting.

Committed d2b4642 and pushed to 8.3.x. Thanks!

Great work, Contrats!

Unfortunately this caused a regression:

WebTestBase used to use $this->originalSite for the original site path while BrowserTestBase used $this->originalSiteDirectory. This is used to determine the path for the settings.testing.php and testing.services.yml files which can be used to influence the child site of the test.

While the reading of this value was consolidated in this issue to always read from $this->originalSite the setting of $this->originalSiteDirectory in BrowserTestBase was not updated accordingly.

Even though this is a regression between 8.2 and 8.3 I would personally propose not rolling this back for the reasons below, but posting the fix here instead of a new issue in case maintainers do think reverting is the best way to go:

• 8.3 does not have a release (although I guess that can change any minute with the nearing alpha, but at least no stable release is near)
• This was a pretty massive undertaking, would be unfortunate to have to re-do work on this
• Not sure if rolling this back would impede/affect #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits in any way
• We cannot add any test coverage for this anyway as this is one of the few places which explicitly interacts with the host system
• The followup is pretty trivial
• The fallout is pretty minor because not many people know about settings.testing.php and testing.services.yml

I can confirm that the patch from #63 solves the test failure we had on our CI system that was due to our settings.testing.php not being found.

Testbot says:
18:25:12 Cannot connect to the Docker daemon. Is the docker daemon running on this host?

Next try.

The patch fixes the regression and the test is green -> RTBC.

Moved to #2849222: settings.testing.php / testing.services.yml not picked up by BrowserTestBase per @alexpott. Re-closing.