Provide a script to install a Drupal testsite

The way this reuses the existing traits is sweet - nice work

1. +++ b/core/scripts/setup-drupal-test.php
   @@ -0,0 +1,273 @@
   +require_once __DIR__ . '/../lib/Drupal/Core/Test/FunctionalTestSetupTrait.php';
   +require_once __DIR__ . '/../tests/Drupal/Tests/RandomGeneratorTrait.php';
   +require_once __DIR__ . '/../tests/Drupal/Tests/SessionTestTrait.php';
   +require_once __DIR__ . '/../lib/Drupal/Core/Test/TestSetupTrait.php';
   

   could we just use $autoload->addPsr4 instead of special casing these four or so? If not, we should document why.

2. +++ b/core/scripts/setup-drupal-test.php
   @@ -0,0 +1,273 @@
   +    $classes = static::fileGetPhpClasses($setup_file);
   

   Should we provide a sample setup file?

3. +++ b/core/scripts/setup-drupal-test.php
   @@ -0,0 +1,273 @@
   +      throw new \InvalidArgumentException(sprintf('You need to define a class implementing \Drupal\Setup\TestSetupInterface'));
   

   A single class?

4. +++ b/core/scripts/setup-drupal-test.php
   @@ -0,0 +1,273 @@
   +    if (!is_subclass_of($classes[0], TestSetupInterface::class)) {
   ...
   +    if (is_subclass_of($classes[0], TestSetupInterface::class)) {
   

   We already throw an exception if the class doesn't implement the interface, so I don't think we need the second check

5. +++ b/core/scripts/setup-drupal-test.php
   @@ -0,0 +1,273 @@
   +class SetupDrupalCommand extends Command {
   ...
   +class SetupDrupalApplication extends Application {
   

   any reason we can't put these in their own files and again use the $autoloader to handle loading. This file would then be just booting the application and running, like you get with app/console in symfony apps

6. +++ b/core/scripts/setup-drupal-test.php
   @@ -0,0 +1,273 @@
   +  ¶
   

   whitespace

7. +++ b/core/scripts/setup-drupal-test.php
   @@ -0,0 +1,273 @@
   +    $test = new FakeTest();
   

   I reckon we should call it TestSiteSetup or somesuch instead of FakeTest, cause although its using the testing traits, its not really a test - to avoid confusion.

8. +++ b/core/scripts/setup-drupal-test.php
   @@ -0,0 +1,273 @@
   +    $output->writeln(drupal_generate_test_ua($test->getDatabasePrefix()));
   

   We could probably give the user something more verbose here, like instructions on how to set the cookie?

9. +++ b/core/tests/Drupal/Setup/TestSetupInterface.php
   @@ -0,0 +1,12 @@
   +  public function setup();
   

   needs docs obviously

Could this be an installer class that can be shared between this use-case and BTB?

Re: Making it a reusable class...

Actually, yah, I was thinking about the wrong thing. The magic ingredient is this:

+++ b/core/scripts/setup-drupal-test.php
@@ -0,0 +1,31 @@
+$kernel = DrupalKernel::createFromRequest($request, $autoloader, 'prod');

'prod' should be 'test' or 'test-for-js' or whatever.

But that's out of scope here. Unfortunately, it will be out of scope everywhere.

Mind I ask a question?

Can we add REST API capital to it? It's ultra useful for sites using another test engines than the CORE one :)

I think it will be cool if it returns SUCCESS install code

I just use 'test', let's see how this work out :)

Of course we currently ignore that setting, which is what I was trying to obliquely point out. :-)

1. +++ b/core/scripts/setup-drupal-test.php
   @@ -0,0 +1,31 @@
   +// Bootstrap.
   +$autoloader = require __DIR__ . '/../../autoload.php';
   

   This comment isn't very helpful.

2. +++ b/core/tests/Drupal/Setup/TestSetupInterface.php
   @@ -0,0 +1,23 @@
   +   * ¶
   

   stray space.

I tried testing this manually, but couldn't get it to work. I ran: "$ php core/scripts/setup-drupal-test.php", but got the error: You must provide a SIMPLETEST_BASE_URL environment variable to run some PHPUnit based functional tests.. I have this in my phpunit.xml: <env name="SIMPLETEST_BASE_URL" value="http://localhost/search-drupal"/>.

Does that mean I'm doing something wrong?

$ ./vendor/bin/phpunit -c core/ --filter SetupDrupalTestScriptTest -v
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

Runtime:	PHP 7.1.1
Configuration:	/Users/paulmitchum/projects/drupal8/core/phpunit.xml

Testing 
R

Time: 24.35 seconds, Memory: 152.00MB

There was 1 risky test:

1) Drupal\FunctionalTests\SetupDrupalTestScriptTest::testInstallScript
This test did not perform any assertions

OK, but incomplete, skipped, or risky tests!
Tests: 1, Assertions: 0, Risky: 1.

Hmm.....

+++ b/core/tests/Drupal/FunctionalTests/SetupDrupalTestScriptTest.php
@@ -0,0 +1,29 @@
+      // @todo How does one test an async API?

https://symfony.com/doc/current/console.html#testing-commands

See also the testbot, such as: http://cgit.drupalcode.org/drupalci_testbot/tree/tests/DrupalCI/Tests/Ap...

ApplicationTester is your friend. You can use it to test against output, and also grab properties from the command object for assertions.

I guess that's a NW...

Shifted things around a little.

The command is now responsible for bootstrapping the kernel.

The test is a TestCase test, run in its own process, to avoid the already-installed exception.

In order to test the state of the installed Drupal, you'd need a way to get the kernel from the console app.

+++ b/core/scripts/setup-drupal-test.php
@@ -0,0 +1,21 @@
+if (PHP_SAPI !== 'cli') {
+  return;
+}

yes, this was what I came back here to check for.

in the middle of the night I had a thought that there may have been a security risk here.

thanks..carry on

Looking at the results, SetupDrupalTestScriptTest didn't run. That's because of #2878269: Modify TestDiscovery so the testbot runs all the tests

Moved the test to be a kernel test because DrupalKernelTest defines drupal_valid_test_ua() all on its own. Kill includes.

The test now uses the test bootstrap.php to get the autoloader, which had to be modified to return the autoloader. This change was an experiment and should be discarded.

IIRC, @neclimdul put that all into drupal_phpunit_populate_class_loader() so we wouldn't have a global called $loader hanging around during testing, which is smart. So this is a WIP and please don't leave it like this.

This patch will fail with an error that looks like this:

1) Drupal\KernelTests\Setup\Commands\SetupDrupalTestScriptTest::testInstallScript
Failed asserting that '
                                                                               
  [PHPUnit_Framework_Error_Warning]                                            
  file_get_contents(/Users/paul/pj2/drupal/sites/simpletest/70212248/.htkey):  
   failed to open stream: No such file or directory                            
                                                                               

setup-drupal-test [--setup_file [SETUP_FILE]] [--db_url [DB_URL]] [--base_url [BASE_URL]]

' does not match PCRE pattern "/PHPUnit_Framework_Error_Warning/".

What is the difference between #2911319: Provide a single command to install & run Drupal and this?

Another thought...

1. +++ b/core/scripts/setup-drupal-test.php
   @@ -0,0 +1,21 @@
   +$app = new TestInstallationSetupApplication();
   +$app->setAutoloader($autoloader);
   
   +++ b/core/tests/Drupal/KernelTests/Setup/Commands/SetupDrupalTestScriptTest.php
   @@ -0,0 +1,63 @@
   +    $app = new TestInstallationSetupApplication(require $autoloader);
   

   I think I prefer passing the autoloader into the constructor since it is definitely required for this to work. That way we could protect the setAutoloader() method and have less public API. Or not ever bother with the setter.

Nice work. Patch looks ready to me some minor observations.

1. +++ b/core/tests/Drupal/KernelTests/Setup/Commands/SetupDrupalTestScriptTest.php
   @@ -0,0 +1,64 @@
   +    $app = new TestInstallationSetupApplication(require $autoloader);
   

   Let move require to separate line.

2. +++ b/core/tests/Drupal/KernelTests/Setup/Commands/SetupDrupalTestScriptTest.php
   @@ -0,0 +1,64 @@
   +    $this->assertNotRegExp('/PHPUnit_Framework_Error_Warning/', $app_tester->getDisplay());
   +    $this->assertNotRegExp('/AlreadyInstalledException/', $app_tester->getDisplay());
   +    $this->assertRegExp('/simpletest/', $app_tester->getDisplay());
   

   Let's use a local var instead of calling the getDisplay again and again.

3. +++ b/core/tests/Drupal/KernelTests/Setup/Commands/SetupDrupalTestScriptTest.php
   @@ -0,0 +1,64 @@
   +    $response = $http_client->send($request);
   +    // Ensure the test_page_test module got installed.
   +    $this->assertContains('Test page | Drupal', (string) $response->getBody());
   

   We should assert response code as well.

4. +++ b/core/tests/Drupal/Setup/Commands/TestInstallationSetupCommand.php
   @@ -0,0 +1,83 @@
   +    $test->setup('testing', $input->getOption('setup_class'));
   

   We should expose profile option as well.

5. +++ b/core/tests/Drupal/Setup/Commands/TestInstallationSetupCommand.php
   @@ -0,0 +1,83 @@
   +  protected function bootstrapDrupal($autoloader) {
   

   Doc block missing.

6. +++ b/core/tests/Drupal/Setup/Commands/TestInstallationSetupCommand.php
   @@ -0,0 +1,83 @@
   +      dirname(dirname(dirname(dirname(__DIR__)))),
   

   Can we use $kernel->getAppRoot() here? Or are we trying to get parent site app root? If that's the case we should add a comment here about it.

7. +++ b/core/tests/Drupal/Setup/TestInstallationSetup.php
   @@ -0,0 +1,150 @@
   +   * @return string
   

   Desc missing.

8. +++ b/core/tests/Drupal/Setup/TestInstallationSetup.php
   @@ -0,0 +1,150 @@
   +    $instance = new $class;
   

   Parentheses missing.

9. +++ b/core/tests/Drupal/Setup/TestSetupInterface.php
   @@ -0,0 +1,23 @@
   +   * modules use
   

   : missing.

1. +++ b/core/tests/Drupal/Setup/Commands/TestInstallationSetupCommand.php
   @@ -0,0 +1,83 @@
   +    $this->bootstrapDrupal($this->autoloader);
   

   Why bother passing this? Couldn't bootstrapDrupal() just use $this->autoloader?

2. +++ b/core/tests/Drupal/Setup/SetupDrupalTestScript.php
   @@ -0,0 +1,19 @@
   + *
   + * @group core
   

   Does this need an @group - it is not a test per-se is it?

3. +++ b/core/tests/Drupal/Setup/TestInstallationSetup.php
   @@ -0,0 +1,150 @@
   +   * @param string $class
   +   *   The setup class.
   

   I think the docs here could be expanded to explain what a set up class is.

4. +++ b/core/tests/Drupal/Setup/TestInstallationSetup.php
   @@ -0,0 +1,150 @@
   +  protected function executeSetupClass($class) {
   

   Should we allow an empty set up class?

   +++ b/core/tests/Drupal/Setup/Commands/TestInstallationSetupCommand.php
   @@ -0,0 +1,83 @@
   +      ->addOption('setup_class', NULL, InputOption::VALUE_OPTIONAL)
   

   It is optional...

+++ b/core/scripts/setup-drupal-test.php
@@ -0,0 +1,20 @@
+require_once __DIR__ . '/../tests/bootstrap.php';

+++ b/core/tests/bootstrap.php
@@ -128,6 +128,7 @@ function drupal_phpunit_populate_class_loader() {
+  $loader->add('Drupal\\Setup', __DIR__);

I'm not 100% convinced about doing this. It means we're registering stuff for things like deprecation testing unnecessarily. Why do we need this? Just for the class loading of the Setup stuff? Can't we just add that to the autoloader ourselves here?

Also I discussed the need to be able to teardown the site - ie. drop the database and remove all the files. Analogous to what happens in \Drupal\Tests\BrowserTestBase::cleanupEnvironment()

If only we had some kind of helper class that does consistent test cleanups for us... #2800267: Turn simpletest_clean*() functions into a helper class

Looks good to me, not much feedback but I don't know much about symfony cli applications.

1. +++ b/core/tests/Drupal/KernelTests/Setup/Commands/SetupDrupalTestScriptTest.php
   @@ -0,0 +1,134 @@
   +    $this->assertRegExp('/simpletest/', $app_tester->getDisplay());
   +    $this->assertEqual(0, $app_tester->getStatusCode());
   +
   +    list($test_db_prefix) = explode(':', $app_tester->getDisplay(), 2);
   

   maybe expand this regex so it actually is on and match better on what you're expecting? Right now it's just an assertContains() ?

2. +++ b/core/tests/Drupal/Setup/TestInstallationSetup.php
   @@ -0,0 +1,192 @@
   +  use FunctionalTestSetupTrait;
   +  use RandomGeneratorTrait;
   +  use SessionTestTrait;
   +  use TestSetupTrait;
   

   pretty awesome that we can re-use all that code directly through traits :)

3. +++ b/core/tests/Drupal/Setup/TestInstallationSetup.php
   @@ -0,0 +1,192 @@
   +        'langcode' => 'en',
   

   in tests we have the ability to install in a different language. I guess we can add this later, might eventually be useful.

Just some nits and stuff, haven't tried running the command yet, will do so when I find some time:

1. +++ b/core/tests/Drupal/KernelTests/Setup/Commands/SetupDrupalTestScriptTest.php
   @@ -0,0 +1,177 @@
   +namespace Drupal\KernelTests\Setup\Commands;
   ...
   + * @todo Move this to the \Drupal\KernelTests\Setup\Commands\ namespace after
   + *   https://www.drupal.org/project/drupal/issues/2878269
   

   Uhh isn't it already in that namespace?

2. +++ b/core/tests/Drupal/KernelTests/Setup/Commands/SetupDrupalTestScriptTest.php
   @@ -0,0 +1,177 @@
   +    // Now test the tear down process as well.
   +    $app_tester->run(
   +      [
   +        'command' => 'teardown-drupal-test',
   +        'db_prefix' => $this->databasePrefix,
   +      ],
   +      [
   +        'interactive' => FALSE,
   +      ]
   +    );
   

   Shouldn't we assert something to test that the tear down worked? Or are we just waiting for exceptions to be thrown?

3. +++ b/core/tests/Drupal/Setup/Commands/TestInstallationSetupCommand.php
   @@ -0,0 +1,88 @@
   +  protected function bootstrapDrupal() {
   

   missing a docblock

4. +++ b/core/tests/Drupal/Setup/Commands/TestTeardownCommand.php
   @@ -0,0 +1,84 @@
   + * Tears down a test Drupal.
   

   "Symfony console command to tear down Drupal." to keep it inline with the set up command?

5. +++ b/core/tests/Drupal/Setup/Commands/TestTeardownCommand.php
   @@ -0,0 +1,84 @@
   +   * Constructs a new TestInstallationSetupCommand.
   

   copy/paste leftover, needs update to the right class

6. +++ b/core/tests/Drupal/Setup/Commands/TestTeardownCommand.php
   @@ -0,0 +1,84 @@
   +  protected function bootstrapDrupal($db_prefix) {
   

   missing docblock

7. +++ b/core/tests/Drupal/Setup/TestInstallationSetup.php
   @@ -0,0 +1,203 @@
   +  protected $timeLimit = 500;
   

   is 500 seconds completely arbitrary or do we have a reason to limit this to 8.3 minutes? It feels like we might hit that limit under normal circumstances, these tests can run a bit long.

8. +++ b/core/tests/Drupal/Setup/TestInstallationSetup.php
   @@ -0,0 +1,203 @@
   +  protected $langcode = 'en';
   

   this is always set during setup() now, so is the default value needed?

9. +++ b/core/tests/Drupal/Setup/TestInstallationSetup.php
   @@ -0,0 +1,203 @@
   +   *   The full qualified class name, which should setup Drupal for tests. One common need for
   

   longer then 80 characters

10. +++ b/core/tests/Drupal/Setup/TestSetupInterface.php
    @@ -0,0 +1,23 @@
    +   * Run code to setup the test.
    

    Run the code to setup the test environment.

11. +++ b/core/tests/Drupal/Setup/TestSetupInterface.php
    @@ -0,0 +1,23 @@
    +   * Check out 'core/tests/Drupal/Setup/SetupDrupalTestScript.php' for an example.
    

    longer then 80 characters

I find the layers of code quite hard to understand in this patch. Everything is very similarly named which does not help. I wonder if refactoring into the relevant part of TestInstallationSetup into TestInstallationSetupCommand and TestTeardownCommand would help. Basically I think TestInstallationSetup is unnecessary and I can't see what it is buying us.

1. +++ b/core/tests/Drupal/KernelTests/Setup/Commands/SetupDrupalTestScriptTest.php
   @@ -0,0 +1,178 @@
   +use Drupal\Core\Database\Connection;
   

   Not used

2. +++ b/core/tests/Drupal/KernelTests/Setup/Commands/SetupDrupalTestScriptTest.php
   @@ -0,0 +1,178 @@
   + * @see \Drupal\Setup\TestInstallationSetup
   
   +++ b/core/tests/Drupal/Setup/Commands/TestInstallationSetupCommand.php
   @@ -0,0 +1,252 @@
   +use Drupal\Setup\TestInstallationSetup;
   
   +++ b/core/tests/Drupal/Setup/Commands/TestTeardownCommand.php
   @@ -0,0 +1,107 @@
   +use Drupal\Setup\TestInstallationSetup;
   

   Not needed - i think

3. +++ b/core/tests/Drupal/KernelTests/Setup/Commands/SetupDrupalTestScriptTest.php
   @@ -0,0 +1,178 @@
   +    $this->assertEqual(0, $app_tester->getStatusCode());
   ...
   +    $this->assertEqual(0, $app_tester->getStatusCode());
   

   assertEquals

4. +++ b/core/tests/Drupal/Setup/Commands/TestInstallationSetupApplication.php
   @@ -0,0 +1,44 @@
   +use Symfony\Component\Console\Input\InputInterface;
   

   Not used.

5. +++ b/core/tests/Drupal/Setup/Commands/TestInstallationSetupCommand.php
   @@ -0,0 +1,252 @@
   +    $this->setName('setup-drupal-test')
   +      ->addOption('setup_class', NULL, InputOption::VALUE_OPTIONAL)
   +      ->addOption('db_url', NULL, InputOption::VALUE_OPTIONAL, 'URL for database or SIMPLETEST_DB', getenv('SIMPLETEST_DB'))
   +      ->addOption('base_url', NULL, InputOption::VALUE_OPTIONAL, 'Base URL for site under test or SIMPLETEST_BASE_URL', getenv('SIMPLETEST_BASE_URL'))
   +      ->addOption('langcode', NULL, InputOption::VALUE_OPTIONAL, 'The language to install the site in.')
   

   Should profile be an option here? Might be useful to choose different testing profiles.

6. +++ b/core/tests/Drupal/Setup/Commands/TestTeardownCommand.php
   @@ -0,0 +1,107 @@
   +  protected function bootstrapDrupal($db_prefix) {
   

   Missing @param doc

7. +++ b/core/tests/Drupal/Setup/TestInstallationSetup.php
   @@ -0,0 +1,20 @@
   +use Drupal\Core\Database\Database;
   +use Drupal\Core\Test\FunctionalTestSetupTrait;
   +use Drupal\Core\Test\TestDatabase;
   +use Drupal\Core\Test\TestSetupTrait;
   +use Drupal\Tests\RandomGeneratorTrait;
   +use Drupal\Tests\SessionTestTrait;
   +
   

   Unused - maybe this entire class is not used?

8. I find the new commands much easier to reason about than the previous abstractions. Nice one.

+++ b/core/tests/Drupal/Setup/Commands/TestInstallationSetupCommand.php
@@ -0,0 +1,253 @@
+  /**
+   * Ensures test files are deletable within file_unmanaged_delete_recursive().
+   *
+   * Some tests chmod generated files to be read only. During
+   * BrowserTestBase::cleanupEnvironment() and other cleanup operations,
+   * these files need to get deleted too.
+   *
+   * @param string $path
+   *   The file path.
+   */
+  public static function filePreDeleteCallback($path) {
+    // When the webserver runs with the same system user as phpunit, we can
+    // make read-only files writable again. If not, chmod will fail while the
+    // file deletion still works if file permissions have been configured
+    // correctly. Thus, we ignore any problems while running chmod.
+    @chmod($path, 0700);
+  }

+++ b/core/tests/Drupal/Setup/Commands/TestTeardownCommand.php
@@ -0,0 +1,110 @@
+    $test_database = new TestDatabase($db_prefix);
+    file_unmanaged_delete_recursive($test_database->getTestSitePath(), [$this, 'filePreDeleteCallback']);
+  }

filePreDeleteCallback function looks like it is in the wrong class - no? Interesting this is not caught by the test.

The teardown command as being called by the test was not working as expected. The patch attached fixes this and improves the test to ensure it is doing what we want it too.

Adding missing docs.

Thank you, for amazing work.

1. Is there any issue/PR where I can see the actual usage of this script?

2. +++ b/core/tests/Drupal/Setup/Commands/TestInstallationSetupCommand.php
   @@ -0,0 +1,233 @@
   +      ->addOption('db_url', NULL, InputOption::VALUE_OPTIONAL, 'URL for database or SIMPLETEST_DB', getenv('SIMPLETEST_DB'))
   +      ->addOption('base_url', NULL, InputOption::VALUE_OPTIONAL, 'Base URL for site under test or SIMPLETEST_BASE_URL', getenv('SIMPLETEST_BASE_URL'))
   ...
   +    putenv("SIMPLETEST_DB=$db_url");
   +    putenv("SIMPLETEST_BASE_URL=$base_url");
   
   +++ b/core/tests/Drupal/Setup/Commands/TestTeardownCommand.php
   @@ -0,0 +1,127 @@
   +      ->addOption('db_url', NULL, InputOption::VALUE_OPTIONAL, 'URL for database or SIMPLETEST_DB', getenv('SIMPLETEST_DB'))
   +      ->addOption('base_url', NULL, InputOption::VALUE_OPTIONAL, 'Base URL for site under test or SIMPLETEST_BASE_URL', getenv('SIMPLETEST_BASE_URL'));
   ...
   +    putenv("SIMPLETEST_DB=$db_url");
   +    putenv("SIMPLETEST_BASE_URL=$base_url");
   

   If the SIMPLETEST_DB and SIMPLETEST_BASE_URL are already set then why are we setting them again? Do you think it is worth doing getenv('SIMPLETEST_DB') first before putenv("SIMPLETEST_DB=$db_url") and don't set them if the values are same?

3. +++ b/core/tests/Drupal/Setup/Commands/TestInstallationSetupCommand.php
   @@ -0,0 +1,233 @@
   +  protected function installParameters() {
   

   Can we mention the reason for overriding this method?

4. I think we need a change notice here
5. I also think we should add a documentation page for all the commands/scripts in core with their sample uses. What do you think? We can create separate issue for that.

Thanks for the review @jibran

1. Yep - it's linked from the issue summary - #2869825: Leverage JS for JS testing (using nightwatch)
2. I don't think the conditionality is worth - it's complexity for no win. Setting environment variables here only affects this process (and potential any process it spawns) so I think it is fine.
3. We can do better here - we can call the trait method to get its handling of database drivers - for example the case were you only have a single db driver available in PHP
4. Agreed we do need a CR
5. I guess you could just create the docs page yourself. Sounds like a good idea.

And here's the patch

Thank you for the fixes. I'll create the documentation page(s).

1. +++ b/core/scripts/setup-drupal-test.php
   @@ -0,0 +1,20 @@
   + * @file
   + * A command line application to install drupal for tests.
   

   We should add a doc block to this file to explain the sample usage and params.

2. +++ b/core/tests/Drupal/Setup/Commands/TestInstallationSetupApplication.php
   @@ -0,0 +1,43 @@
   + * Application wrapper for TestInstallationSetupCommand.
   

   Should we explain the commands for this application?

3. +++ b/core/tests/Drupal/Setup/Commands/TestInstallationSetupCommand.php
   @@ -0,0 +1,200 @@
   + * Symfony console command to setup Drupal.
   

   I think we should improve the class docs here as well and explain the things this class is doing e.g. installs a Drupal testing site and execute setup class.

4. +++ b/core/tests/Drupal/Setup/Commands/TestInstallationSetupCommand.php
   @@ -0,0 +1,200 @@
   +   * Uses the setup file to configure Drupal.
   

   I know we have explained the param but we should also improve the function doc block. We are also missing @throw here.

5. +++ b/core/tests/Drupal/Setup/Commands/TestInstallationSetupCommand.php
   @@ -0,0 +1,200 @@
   +      throw new \InvalidArgumentException("There was a problem loading {$class}");
   ...
   +      throw new \InvalidArgumentException(sprintf('You need to define a class implementing \Drupal\Setup\TestSetupInterface'));
   

   Why are we using sprintf in one and not the other?

6. +++ b/core/tests/Drupal/Setup/Commands/TestInstallationSetupCommand.php
   @@ -0,0 +1,200 @@
   +    $parameters = $this->installParametersTrait();
   

   Why can't this be just parent call?

7. +++ b/core/tests/Drupal/Setup/Commands/TestTeardownCommand.php
   @@ -0,0 +1,127 @@
   + * Symfony console command to tear down Drupal.
   

   Again we should add more docs to this class explaining the functionality.

8. +++ b/core/tests/Drupal/Setup/Commands/TestTeardownCommand.php
   @@ -0,0 +1,127 @@
   +   * Some tests chmod generated files to be read only. During
   

   What do we mean here by 'chmod generated'? Can we please use proper/complete words?

9. +++ b/core/tests/Drupal/Setup/Commands/TestTeardownCommand.php
   @@ -0,0 +1,127 @@
   +    @chmod($path, 0700);
   

   Aren't we planning to deprecate the usage of @ to suppress the warnings with an empty catch?

10. +++ b/core/tests/Drupal/Setup/Commands/TestTeardownCommand.php
    @@ -0,0 +1,127 @@
    +      dirname(dirname(dirname(dirname(__DIR__)))),
    

    I think I pointed this earlier as well, can we use $kernal->getAppRoot() here?

11. +++ b/core/tests/Drupal/Setup/SetupDrupalTestScript.php
    @@ -0,0 +1,17 @@
    + * Setup file used by \Drupal\KernelTests\Setup\Commands\SetupDrupalTestScriptTest
    

    > 80 char.

12. +++ b/core/tests/Drupal/Setup/TestSetupInterface.php
    @@ -0,0 +1,24 @@
    +   * You have access to any API provided by any installed module. To install
    

    I think it should be
    You have access to any API provided by any installed module. For example to install

13. The is not php core/scripts/setup-drupal-test.php anymore so we need IS update

With PHP 7.2.2 and phpunit 6.5.6 when I run php core/scripts/setup-drupal-test.php setup-drupal-test I'm getting the following warning.

PHP Warning:  ini_set(): Headers already sent. You cannot change the session module's ini settings at this time in /data/app/core/lib/Drupal/Core/DrupalKernel.php on line 980
PHP Stack trace:
PHP   1. {main}() /data/app/core/scripts/setup-drupal-test.php:0
PHP   2. Drupal\Setup\Commands\TestInstallationSetupApplication->run() /data/app/core/scripts/setup-drupal-test.php:20
PHP   3. Drupal\Setup\Commands\TestInstallationSetupApplication->doRun() /data/app/vendor/symfony/console/Application.php:148
PHP   4. Drupal\Setup\Commands\TestInstallationSetupApplication->doRunCommand() /data/app/vendor/symfony/console/Application.php:248
PHP   5. Drupal\Setup\Commands\TestInstallationSetupCommand->run() /data/app/vendor/symfony/console/Application.php:946
PHP   6. Drupal\Setup\Commands\TestInstallationSetupCommand->execute() /data/app/vendor/symfony/console/Command/Command.php:252
PHP   7. Drupal\Setup\Commands\TestInstallationSetupCommand->bootstrapDrupal() /data/app/core/tests/Drupal/Setup/Commands/TestInstallationSetupCommand.php:104
PHP   8. Drupal\Core\DrupalKernel::createFromRequest() /data/app/core/tests/Drupal/Setup/Commands/TestInstallationSetupCommand.php:120
PHP   9. Drupal\Core\DrupalKernel::bootEnvironment() /data/app/core/lib/Drupal/Core/DrupalKernel.php:269
PHP  10. ini_set() /data/app/core/lib/Drupal/Core/DrupalKernel.php:980

Patch attached moves the test to a unit test and fixes things so that the test works on MySQL, SQLite and Postgres regardless of what is configured in the parent site's settings.php.

#70 is still to be addressed.

Fixing the tests and PHP7.2 too.

Still to address #70 and #72.

Re #74

1. This code is called from TestInstallationSetupCommand which does not have a $this->originalContainer so an instanceof check will still cause an error. The only possible types for $this->originalContainer are NULL - the default value in most test bases, not set at all - our command or the container as it becomes in most test bases. I think we need the isset() check here or to add the property the will never be used to TestInstallationSetupCommand. I prefer the cleanliness of checking if it isset because it is not defined by the trait.
2. That change is no longer - it was breaking things and I found another way around the problem it was fixing in #74
3. Added
4. Indeed it is
5. I don't understand this is a copy of what is in \Drupal\Tests\BrowserTestBase::cleanupEnvironment()
6. All we can do is test we've create no additional tables.
7. Yeah it's a good thing to know we've not obliterated everything!

re #70

1. I've fixed up the commands to make better use of Console's help system. I don't think we should duplicate this in class docblocks. I've tweaked them to be clearer and point to how to get help from console.
2. See 1.
3. See 1.
4. This function no longer throws and is way simpler
5. Yep there's no need for the sprintf - fixed
6. Because the method is not from the parent it is from the trait and we rename it when we use the trait.
7. See 1
8. This is copy&paste from the existing test system - see \Drupal\simpletest\TestBase::filePreDeleteCallback I don't think we need to change it here. However we can just use the version from BrowserTestBase. So fixed - no longer exists here :)
9. See 8. Also we can't do that in PHP 5
10. No longer exists
11. Fixed
12. Fixed
13. php core/scripts/setup-drupal-test.php is still the application - it has two commands and it tells you about them.
14. PHP7.2 is now fixed

Also fixed "teardown" not being a word recognised by PHPStorm - so tear-down and tearDown are preferred.

The change record exists https://www.drupal.org/node/2943615

Patch attached completely refactors all the class names and namespaces to be consistent. The basic idea is that the word Drupal is redundant. The application is a TestSite application with two commands - install and tear-down. Updated the issue summary and change record accordingly. No interdiff because it will be way way bigger than the patch and probably confusing. The reason things got inconsistent was that the first command was just to install a site - we then added a tear down command and didn't refactor all the stuff around it.

See issue summary for updated commands.

Ensure that both commands now output at least something to tell the user what has happened. The install command now has a new option to output in json - --json.

+++ b/core/tests/Drupal/TestSite/TestSetupInterface.php
@@ -0,0 +1,25 @@
+ * Allows you to setup an environment used for javascript tests.

Suggests this will/should only be used for javascript tests, whereas the naming is more generic.

@jonathanshaw good point. We don't need to specify exactly what the test site might be used for. Javascript testing is the initial aim but the script could be used for all sorts of testing where the test-runner / specification is less tied to Drupal's code.

Fix a typo.

Nice tidy ups @dawehner

re #83

1. I don't think we need to make the command internal - there is no really API exposed for people to build on and I think this command might turn out useful in scenarios other than core JS testing. Also we have the version number to declare the command's version. See 4...
2. I dunno but only outputting JSON felt weird. I'd prefer to leave this as is and see what people come up with. Especially since we're not exposing API but I think we should agree to keep the commands and options the same so people can build testing tools on top of this. Once we've reached version 1 :)
3. Me neither. I think this is copy/pasta from \Drupal\Tests\BrowserTestBase::cleanupEnvironment - it is unnecessary.
4. I agree. Interestingly, maybe this should be \Drupal::VERSION?
5. Yep we could - in fact this was fixed in #83 - nice.

1. +++ b/core/tests/Drupal/TestSite/Commands/TestSiteInstallCommand.php
   @@ -0,0 +1,203 @@
   +
   +
   

   nit: extra line

2. +++ b/core/tests/Drupal/TestSite/Commands/TestSiteInstallCommand.php
   @@ -0,0 +1,203 @@
   +      ->addOption('json', NULL, InputOption::VALUE_NONE, 'Output test site connection details in JSON');
   

   Can we add a few ->addUsage examples here

3. +++ b/core/tests/Drupal/TestSite/Commands/TestSiteInstallCommand.php
   @@ -0,0 +1,203 @@
   +    if (!is_subclass_of($class, TestSetupInterface::class)) {
   

   any reasons not to use class_implements here? given its an interface and not a class?

4. +++ b/core/tests/Drupal/TestSite/Commands/TestSiteTearDownCommand.php
   @@ -0,0 +1,118 @@
   +      ->addOption('db_url', NULL, InputOption::VALUE_OPTIONAL, 'URL for database or SIMPLETEST_DB', getenv('SIMPLETEST_DB'))
   +      ->addOption('base_url', NULL, InputOption::VALUE_OPTIONAL, 'Base URL for site under test or SIMPLETEST_BASE_URL', getenv('SIMPLETEST_BASE_URL'));
   

   same here, a couple of addUsage examples would be useful

5. +++ b/core/tests/Drupal/TestSite/Commands/TestSiteTearDownCommand.php
   @@ -0,0 +1,118 @@
   +    $db_prefix = $input->getArgument('db_prefix');
   

   should we validate this - could someone possibly pass an empty string? These all used to be /^simpletest([0-9]+)$/ - should we check that here to prevent people dropping the wrong tables?

6. +++ b/core/tests/Drupal/Tests/Scripts/TestSiteApplicationTest.php
   @@ -0,0 +1,215 @@
   +class TestSiteApplicationTest extends UnitTestCase {
   

   <3 this is some damn fine work

Thanks for the review @larowlan

1. Fixed
2. Done
3. I prefer is_subclass_of() because with class_implements you have to use in_array() or something similar. Seems more complex?
4. Done - made me realise the db_url option is useless for teardown.
5. Yes validation is a very good idea considering we're deleting tables. We already were because of using the TestDabase class but made it more explicit and helpful. Plus it is tested now too.

#86 is addressed so back to RTBC.

+1 from this side. I ran

$ SIMPLETEST_DB=sqlite://localhost/sites/default/files/.ht.sqlite SIMPLETEST_BASE_URL=http://localhost:8080 php ./core/scripts/test-site.php install

with output

Successfully installed a test site
 ----------------- --------------------------------------------------------------------------------------------------- 
  Database prefix   test84433263                                                                                       
  User agent        simpletest84433263:1518997804:5a8a112c8e0f57.53253374:6Z5iz-mWTeuAoccTRLH8LAIiwDfu3_M3KtQpMG-nI_E  
 ----------------- --------------------------------------------------------------------------------------------------- 

and

$ SIMPLETEST_DB=sqlite://localhost/sites/default/files/.ht.sqlite SIMPLETEST_BASE_URL=http://localhost:8080 php ./core/scripts/test-site.php install --json
<code>

with

<code>
{"db_prefix":"test99024012","user_agent":"simpletest99024012:1518997845:5a8a1155b66937.93128468:PuycM-nrKjtwmd2Vf-WH0BJsSwRtMdQrHzur0_TaIHw"}

I have to board a plane, but will test tear-down on plane.

I kept getting errors. I'm not sure if it happens to be my setup, so I'm not wanting to be the person who kicks it back to Needs Work. But I kept receiving the error " The specified database connection is not defined: default "

The command tests the prefix, but there are errors later in the bootstrap process. It seems to be in drupal_valid_test_ua where it falls back to default because it cannot detect the proper UA. So it seems like we need the UA passed and not the test prefix because the existing process should detect the prefix and validate it via the UA.

EDIT: Command w/ output

$ SIMPLETEST_BASE_URL=http://localhost:8080 SIMPLETEST_DB=sqlite://localhost/sites/default/files/.ht.sqlite php ./core/scripts/test-site.php tear-down test64245371 --db_url=sqlite://localhost/sites/default/files/.ht.sqlite

In Database.php line 361:
                                                             
  The specified database connection is not defined: default  

EDIT 2: This diff of change worked, ensuring there was a UA

@@ -116,6 +116,7 @@ protected function tearDown(TestDatabase $test_database, $db_url, $app_root) {
    *   The Drupal kernel.
    */
   protected function bootstrapDrupal() {
+    $_SERVER['HTTP_USER_AGENT'] = 'simpletest40291844:1519006069:5a8a3175a23554.35509240:ZrLAwUqPWmeq9zlogYrsUIkkrK4bznFXRdNRaLYIc6E';
     $request = Request::createFromGlobals();

I could not get it work despite what I did, I had to use the user agent, which follows the normal boot process.

@mglaman thanks for testing the command.

I can't reproduce what you are seeing in #91 with #87 :(

Ah I've got it. It's because @mglaman didn't have a valid Drupal installation so no settings.php. Which is a great way to test this command as it should work in those conditions. Unfortunately I cannot think of a way to test this via run-tests.sh The interesting question is why does the testbot pass? I think this is because a default connection gets defined. I guess this suggests a better fix more internal to the tear-down command. We can set the drupal_valid_test_ua() prefix here and not have a pretend request at all.

@mglaman's test environment helped yet again. If you have a sites/default/settings.testing.php you'd get issues with DRUPAL_ROOT not being defined. Here's the fix. Again not sure how to test this but the command is now more robust. It works both with and without a sites/default/settings.testing.php and sites/default/settings.php.

Let's just decouple test site tear down from being able to bootstrap the Drupal test site. This means that the test could break Drupal completely and the tear down still be successful. Which seems like a good idea to me.

I ran through #96 locally a few times. Works like a charm without errors, and cleans up!

[11:29 AM]-[mglaman@Matts-MBP]-[~/Drupal/sites/drupal8/web/sites/simpletest]-[git 2926633] 
$ ls | grep 65475271
65475271

[11:29 AM]-[mglaman@Matts-MBP]-[~/Drupal/sites/drupal8/web/sites/simpletest]-[git 2926633] 
$ ls | grep 65475271

And the SQLite file was removed (only had ones from previous errors

[11:31 AM]-[mglaman@Matts-MBP]-[~/Drupal/sites/drupal8/web/sites/default/files]-[git 2926633] 
$ ls -la
total 14920
drwxr-xr-x  13 mglaman  staff      416 Feb 19 11:30 .
drwxrwxr-x   9 mglaman  staff      288 Feb 19 11:29 ..
-rw-r--r--   1 mglaman  staff     4096 Feb 18 20:07 .ht.sqlite
-rw-r--r--   1 mglaman  staff  1351680 Feb 18 20:23 .ht.sqlite-test13477502
-rw-r--r--   1 mglaman  staff  1105920 Feb 18 20:25 .ht.sqlite-test21321087
-rw-r--r--   1 mglaman  staff  1785856 Feb 18 20:24 .ht.sqlite-test32828154
-rw-r--r--   1 mglaman  staff     8192 Feb 18 20:33 .ht.sqlite-test43484065
-rw-r--r--   1 mglaman  staff     8192 Feb 18 20:19 .ht.sqlite-test57714316
-rw-r--r--   1 mglaman  staff        0 Feb 18 20:38 .ht.sqlite-test63595005
-rw-r--r--   1 mglaman  staff  1351680 Feb 18 20:19 .ht.sqlite-test63963458
-rw-r--r--   1 mglaman  staff     8192 Feb 18 20:35 .ht.sqlite-test68264670
-rw-r--r--   1 mglaman  staff     8192 Feb 18 20:29 .ht.sqlite-test70553203
drwxrwxrwx   2 mglaman  staff       64 Feb 18 20:07 simpletest

[11:31 AM]-[mglaman@Matts-MBP]-[~/Drupal/sites/drupal8/web/sites/default/files]-[git 2926633] 
$ ls -la | grep 65475271

Going to pull the RTBC trigger. The initial bugs I found have been addressed by alexpott. It's also more stable around general failures that could occur and prevent clean up.

@dawehner I dunno that all feels a little bit of a wishful @todo. If we do introduce either of those it'll be the job of that work to do replace things like file_unmanaged_delete_recursive() and we have an @see here so I think we're good.

In testing with https://github.com/jsdrupal/nightwatch I've discovered one area of complexity. This command requires simpletest/sites to exist and to be writable. If the command is run as a user that can create this then it does so all good. But in reality we need the command to be run by the user running the webserver so I think we should be a bit more defensive and helpful here because people are likely to see an exception.

Here's a fix for that. This is not testable unfortunately because we can't use a virtual filesystem :(

Here's what happens if the command can't write or create sites/simpletest:

#103 is a good catch, bumping back to RTBC.

There isn't a good way to test it, and it is a good use case. We currently have the following in our CircleCI config because Functional tests fail when it isn't writeable and I think I recall it having a similar message.

          command: |
            mkdir -p sites/simpletest

Realised one thing. When we create a test site if RUN_TESTS_CONCURRENCY is greater than 1 we create a lock file so that no other test site gets the same db prefix. So if we do concurrent testing under nightwatch, and that takes place outside of run-tests.sh, we're going to need to handle this. Both to use the locking solution and somehow clear it up. Given we're not aiming for that here not sure we need to do anything on this issue. But it felt important to note so that others know.

So if we do concurrent testing under nightwatch, and that takes place outside of run-tests.sh

It might be that we'll need some kind of concurrency handler for nightwatch tests like run-tests.sh does for php tests.

@Mixologic well we're still using the TestDatabase class so as long as this sets RUN_TESTS_CONCURRENCY to something greater than one than we'll get locking and ensured uniqueness. So #105 is more about just keeping people informed that we need to think about setting that in concurrent situations. And think about tearing it down. That said we could implement a --lock file flag on the commands to lock and clean up the lock.

Patch attached addresses a weakness @dawehner noticed. We should ensure we only clean up stuff inside of drupal root.

Been thinking about the locking stuff a bit more. I think this command should use a different database prefix from the current test12345678. And it should always create a lock.

Here's a solution with locking in place. This will allow Nightwatch to use multiple workers and we can clean up all the locks at the end. I think it is important that we follow run-tests.sh example and not re-use prefixes during tests since this further isolates tests from weird random fails. It does mean that run-tests.sh and Nightwatch should not run in parallel with each other but I think this is acceptable.

Also we can completely refactor the commands and application to never use the autoloader - which in my opinion is nice.

Discussed further with @dawehner we agreed to file to explore always locking. Created #2946439: TestDatabase should always lock to follow this up.

@dawehner so add an option to teardown to clear the lock. Makes sense. Still means we can't test releasing all the locks but that's okay I guess.

Implemented #112. Now when tearing down you have an option to keep the lock. So the default behaviour is as expected ie it'll remove the lock but if you are implementing concurrent testing and you want to ensure isolation you can keep the locks after tear down.

@dawehner nice finds. I don't think I can rtbc this myself since many of the changes since the last rtbc are mine but I think we've proved over on the Nightwatch branch this is works. Yes there are additions to make like the support for installing modules without needing a custom script but these will be better to be teased out when we actually write Nightwatch tests rather than guessing upfront. At least with the script we can do any setup tasks required.

I agree with #116.

Also, its *really* quite quick. I was surprised when running the nightwatch test that it ran in < 5 sec. Thats fantastic.

I like this. There are definitely other ways to improve, as stated in #116. But I think those need to be solved by use cases.

The test script isn't even horrible, in my opinion. I experimented with this patch and Nightwatch testing and the test script results are here: https://github.com/mglaman/drupal-commerce-nightwatch/tree/commerce-expe...

Worked like a charm.

+1!

1. +++ b/core/lib/Drupal/Core/Test/TestDatabase.php
   @@ -62,13 +62,17 @@ public static function getConnection() {
   +   * @param bool $create_lock
   +   *   (optional) Whether or not to create a lock file. Defaults to FALSE. If
   +   *   the environment variable RUN_TESTS_CONCURRENCY is greater than 1 it will
   +   *   be overridden to TRUE regardless of its initial value.
   
   @@ -107,31 +111,48 @@ public function getDatabasePrefix() {
   +  protected function getTestLock($create_lock = FALSE) {
   +    // If we're only running with a concurrency of greater than 1 there is a
   +    // risk the random number generated clashing. Therefore we need to create
   +    // a lock.
   +    if (getenv('RUN_TESTS_CONCURRENCY') > 1) {
   +      $create_lock = TRUE;
   +    }
   

   This is confusing. Can we document why it needs to work this way?

2. +++ b/core/lib/Drupal/Core/Test/TestDatabase.php
   @@ -107,31 +111,48 @@ public function getDatabasePrefix() {
   diff --git a/core/scripts/test-site.php b/core/scripts/test-site.php
   
   diff --git a/core/scripts/test-site.php b/core/scripts/test-site.php
   new file mode 100644
   

   Shouldn't this be executable?

3. +++ b/core/lib/Drupal/Core/Test/TestDatabase.php
   --- /dev/null
   +++ b/core/scripts/test-site.php
   

   ❤️

4. +++ b/core/scripts/test-site.php
   @@ -0,0 +1,22 @@
   + * A command line application to install drupal for tests.
   

   Nit: s/drupal/Drupal/. Fixed.

5. +++ b/core/scripts/test-site.php
   @@ -0,0 +1,22 @@
   +$app = new TestSiteApplication('test-site', '0.1.0');
   

   What's this '0.1.0'?

   This also shows up when executing it:

   ~/Work/d8 8.6.x* 💭  php core/scripts/test-site.php 
   test-site 0.1.0
   
   Usage:
     command [options] [arguments]
   
   Options:
     -h, --help            Display this help message
     -q, --quiet           Do not output any message
     -V, --version         Display this application version
         --ansi            Force ANSI output
         --no-ansi         Disable ANSI output
     -n, --no-interaction  Do not ask any interactive question
     -v|vv|vvv, --verbose  Increase the verbosity of messages: 1 for normal output, 2 for more verbose output and 3 for debug
   
   Available commands:
     help           Displays help for a command
     install        Creates a test Drupal site
     list           Lists commands
     release-locks  Releases all test site locks
     tear-down      Removes a test site added by the install command
   

   Other than that, ❤️ this script and its output!

6. +++ b/core/tests/Drupal/TestSite/Commands/TestSiteInstallCommand.php
   @@ -0,0 +1,231 @@
   +use Symfony\Component\Console\Command\Command;
   ...
   +class TestSiteInstallCommand extends Command {
   

   WOAH, TIL! 👀

7. +++ b/core/tests/Drupal/TestSite/Commands/TestSiteInstallCommand.php
   @@ -0,0 +1,231 @@
   +  /**
   +   * Time limit in seconds for the test.
   +   *
   +   * @var int
   +   */
   +  protected $timeLimit = 500;
   

   This is not used anywhere? Not sure if it can be removed, or if some code needs to be added.

8. +++ b/core/tests/Drupal/TestSite/Commands/TestSiteInstallCommand.php
   @@ -0,0 +1,231 @@
   +      ->addUsage('--install_profile demo_umami --langcode fr')
   

   Nice!

9. +++ b/core/tests/Drupal/TestSite/Commands/TestSiteInstallCommand.php
   @@ -0,0 +1,231 @@
   +  protected function validateSetupClass($class) {
   

   ❤️ strict validation, that verifies the script's assumptions are met, thus helping to prevent a developer from getting confused and having to dig into details!

10. +++ b/core/tests/Drupal/TestSite/Commands/TestSiteInstallCommand.php
    @@ -0,0 +1,231 @@
    +   *   The full qualified class name, which should setup Drupal for tests. For
    

    Nits:
    - s/full/fully/
    - s/setup/set up/

    Fixed.

11. +++ b/core/tests/Drupal/TestSite/Commands/TestSiteReleaseLocksCommand.php
    @@ -0,0 +1,34 @@
    +class TestSiteReleaseLocksCommand extends Command {
    

    Missing test coverage. I think we could test this at the end of \Drupal\Tests\Scripts\TestSiteApplicationTest::testInstallScript()?

    Done.

12. +++ b/core/tests/Drupal/TestSite/Commands/TestSiteTearDownCommand.php
    @@ -0,0 +1,133 @@
    +   * Deletes all files and directories in the specified filepath recursively.
    

    Nit: s/filepath/path/. Fixed.

13. +++ b/core/tests/Drupal/TestSite/Commands/TestSiteTearDownCommand.php
    @@ -0,0 +1,133 @@
    +   * Note this version has no dependencies on Drupal core to ensure that the
    

    I don't understand "this version"?

14. +++ b/core/tests/Drupal/TestSite/Commands/TestSiteTearDownCommand.php
    @@ -0,0 +1,133 @@
    +   * @param $path
    

    Nit: Missing string typehint. Fixed.

15. +++ b/core/tests/Drupal/TestSite/TestSetupInterface.php
    @@ -0,0 +1,27 @@
    + * Allows you to setup an environment as part of a test site install.
    

    Nit: we avoid "you" in code comments AFAIK. Fixed.

16. +++ b/core/tests/Drupal/Tests/Scripts/TestSiteApplicationTest.php
    @@ -0,0 +1,267 @@
    + * Therefore run in an separate process to avoid side effects.
    

    Supernit: s/an/a/. Fixed.

17. +++ b/core/tests/Drupal/Tests/Scripts/TestSiteApplicationTest.php
    @@ -0,0 +1,267 @@
    +class TestSiteApplicationTest extends UnitTestCase {
    

    ❤️

18. +++ b/core/tests/Drupal/Tests/Scripts/TestSiteApplicationTest.php
    @@ -0,0 +1,267 @@
    +   * @coversNothing
    

    TIL!

19. +++ b/core/tests/Drupal/Tests/Scripts/TestSiteApplicationTest.php
    @@ -0,0 +1,267 @@
    +    $http_client = new Client();
    +    $request = (new Request('GET', getenv('SIMPLETEST_BASE_URL') . '/test-page'))
    +      ->withHeader('User-Agent', trim($result['user_agent']));
    +
    +    $response = $http_client->send($request);
    +    // Ensure the test_page_test module got installed.
    +    $this->assertContains('Test page | Drupal', (string) $response->getBody());
    

    ❤️

20. +++ b/core/tests/Drupal/Tests/Scripts/TestSiteApplicationTest.php
    @@ -0,0 +1,267 @@
    +    // Ensure that there are files and database tables for tear down command to
    

    Nit: s/for tear/for the tear/

    Fixed.

21. +++ b/core/tests/Drupal/Tests/Scripts/TestSiteApplicationTest.php
    @@ -0,0 +1,267 @@
    +    // Install another site so we can ensure tear down only removes one site at
    

    Nit: s/ensure tear down only/ensure the tear down command only/

    Fixed.

22. +++ b/core/tests/Drupal/Tests/Scripts/TestSiteApplicationTest.php
    @@ -0,0 +1,267 @@
    +    // Now test the tear down process as well.
    +    $command_line = $php_binary_path . ' core/scripts/test-site.php tear-down ' . $db_prefix . ' --keep_lock --db_url "' . getenv('SIMPLETEST_DB') . '"';
    ...
    +    // Ensure that all the tables and files for this DB prefix are gone.
    ...
    +    // Ensure the other site's tables and files still exist.
    

    ❤️

    Missing the fact that we're explicitly keeping the lock during the tear down. Fixed.

23. +++ b/core/tests/Drupal/Tests/Scripts/TestSiteApplicationTest.php
    @@ -0,0 +1,267 @@
    +    // Tear down the other site installed. Tear down should work if the test
    

    Nit: s/other site installed/other site/. Fixed.

24. +++ b/core/tests/Drupal/Tests/Scripts/TestSiteApplicationTest.php
    @@ -0,0 +1,267 @@
    +    // The lock for the first site should still exist but the second site is
    +    // clean up during tear down.
    

    Nit: s/is clean up during/'s lock is released during/. Fixed.

re #122.11 we can't test that because you'll delete all the real test locks :) and potentially cause random fails.

+++ b/core/tests/Drupal/Tests/Scripts/TestSiteApplicationTest.php
@@ -173,10 +173,23 @@ public function testInstallScript() {
+    // Release all locks.
+    $command_line = $php_binary_path . ' core/scripts/test-site.php release-locks';
+    $process = new Process($command_line, $this->root);
+    // Set the timeout to a value that allows debugging.
+    $process->setTimeout(500);
+    $process->run();
+    $this->assertSame(0, $process->getExitCode());
+    $this->assertContains("Successfully released all the test database locks", $process->getOutput());
+
+    // Both tests sites' locks should no longer exist now.
+    $this->assertFileNotExists($this->getTestLockFile($db_prefix));
+    $this->assertFileNotExists($this->getTestLockFile($other_db_prefix));

Unfortunately this need to go :(

So we also need to address #122 1, 2, 5, 7, 13

Re 1. I'm not sure what more you want here. It has to work this way because it is being using by DrupalCI / run-test.sh to run with a concurrency of 31.
Re 2. I dunno for me I always run it like php core/scripts/test-site.php install that does not need it to be executable.
Re 5. It's the command version - we set it to 0.1.0 because this is an internal test running command and not part of the API until we can a chance to use it properly.
Re 7. It's used in \Drupal\Core\Test\FunctionalTestSetupTrait::prepareEnvironment - i.e. by one of the traits - it's not defined there for legacy annoying reasons.
Re 13. version I guess was about this version of this method ... but it makes more sense when version is replaced with method. This is as opposed to file_unmanaged_delete_recursive() which does have dependencies on Drupal and it's container.

Addressed #124 and #122.1 (hopefully), 5, 7, and 13.

I'm not sure what more you want here. It has to work this way because it is being using by DrupalCI / run-test.sh to run with a concurrency of 31.

I think it's very confusing that we accept a parameter to determine behavior, but then sometimes choose to ignore that parameter anyway. I think it'd then be better to NOT have a parameter, and just rely on getenv(…) always.

RE: 2: ok, w/e for now. It's inconsistent already in our existing scripts: some are executable, some are not.

Everything else, and all patch changes make sense. 👍

Unfortunately this need to go :(

Can we then at least document either on the command itself or in the place where the test coverage was just removed why we cannot test this?

@Wim Leers the problem is we're not setting concurrency here (it's a run-tests.sh concept) but we need locking. #2946439: TestDatabase should always lock addresses the issue and will make it simpler.

+++ b/core/tests/Drupal/TestSite/Commands/TestSiteReleaseLocksCommand.php
@@ -10,6 +10,9 @@
+ * Note that this command can't be safely tested by DrupalCI without potentially
+ * causing random failures.

Can we then at least document either on the command itself or in the place where the test coverage was just removed why we cannot test this?

Did that already...

Did that already...

Sorry, overlooked that!

Back to RTBC 👍

Also, its *really* quite quick. I was surprised when running the nightwatch test that it ran in < 5 sec. Thats fantastic.

Can't wait!

Changed --setup_class to --setup_file and added testing around the new expectation of that being a file that contains a class.

Last failure looks like a FunctionalJavascript Randofail. Requeued.

Nice fix @dawehner.

Over in #2911319: Provide a single command to install & run Drupal we've standardised on - instead of _ and I think this makes sense. It is the standard for console commands. It is used by composer and phpcs and drush.

Leaving at rtbc because this is a cosmetic change.

Unrelated fails

Agreed.

+++ b/core/tests/Drupal/TestSite/Commands/TestSiteTearDownCommand.php
@@ -0,0 +1,133 @@
+    $output->writeln("<info>Successfully uninstalled $db_prefix test site</info>");

Took this for a test drive, only question - should we have a --json option for this too?

@larowlan re json output - we don't need one atm - can always add one later if we need it. But there's nothing to consume in terms of info. If you want to know whether a tear down is successful - you can check the exit status.

adding review credits

Committed to 8.6.x and published change notice.

Thanks

One test written for this issue is causing a build failure for the D8 Composer template (drupal-composer/drupal-project). As you can see, the failures started happening right after the file referenced below was committed.

Here is the PHPUnit output:

There was 1 failure:
1) Drupal\Tests\Scripts\TestSiteApplicationTest::testInstallWithNonSetupClass
Failed asserting that '
In TestSiteInstallCommand.php line 149:
                                                                               
  The class Drupal\Tests\Scripts\TestSiteApplicationTest contained in /home/t  
  ravis/build/drupal-composer/drupal-project/web/core/tests/Drupal/Tests/Scri  
  pts/TestSiteApplicationTest.php needs to implement \Drupal\TestSite\TestSet  
  upInterface                                                                  
                                                                               

install [--setup-file [SETUP-FILE]] [--db-url [DB-URL]] [--base-url [BASE-URL]] [--install-profile [INSTALL-PROFILE]] [--langcode [LANGCODE]] [--json]

' contains "needs to implement \Drupal\TestSite\TestSetupInterface".

/home/travis/build/drupal-composer/drupal-project/web/core/tests/Drupal/Tests/Scripts/TestSiteApplicationTest.php:96

The desired string cannot be found in that output because the output contains a line break and extra spaces in the middle of it, throwing it off.

I created a issue for it. #2960214: TestSiteApplicationTest fails on sites with long base path

Thanks :)

Little bit of a follow-up: #2962157: TestSiteApplicationTest requires a database despite being a unit test