TestSiteApplicationTest requires a database despite being a unit test

Let's just mark the test skipped if it is not present. If we want to test our test infrastructure we need to allow these types of tests.

Nice find @Mile23 and woah not fair testbot.

So the counts in the tests where we're not doing an install are impossible in a concurrent test env where the db is shared so lets remove them.

What #11 should have been.

More instrumentation.

#15 was about finding out what was wrong! No need to include that :)

This might fix it.

RTBC++

Why not just call inheritEnvironmentVariables() like it tells us to?

The additional env variables for start and run are just when you want to change the default environment you've set it up with. Constructing and running with the same $env is wrong.
See https://github.com/symfony/symfony/blob/bb4bde3bba9a8f8cd209855cb5589d10...
and https://github.com/symfony/symfony/blob/bb4bde3bba9a8f8cd209855cb5589d10...

+++ b/core/tests/Drupal/Tests/Core/Scripts/TestSiteApplicationTest.php
@@ -183,14 +142,15 @@ public function testInstallScript() {
+    $process = new Process($command_line, $this->root, $this->env);
     // Set the timeout to a value that allows debugging.
     $process->setTimeout(500);
-    $process->run();
+    $process->run(NULL, $this->env);

The other option here is to not pass $this->env to the constructor. But personally I don't like looking at $process->run(NULL, $this->env);. But's that's a taste thing.

@Mile23 I think we'll find this is not that simple because of how PHPUnit.xml.dist configures environment variables especially in PHPUnit 4.

So looking at Process again here's a patch that just passes the environment on to Process::run() I'm not a huge fan of $process->run(NULL, $this->env); but calling Process::inheritEnvironmentVariables() also seems silly because in symfony 4 that method is totally redundant. So this feels the best way to not use deprecated functionality.

1. +++ b/core/tests/Drupal/Tests/Core/Scripts/TestSiteApplicationTest.php
   @@ -262,13 +226,16 @@ public function testTearDownDbPrefixValidation() {
   -    $target = __CLASS__ . $db_prefix;
   -    Database::addConnectionInfo($target, 'default', $database);
   -    return $target;
   +    $key = __CLASS__ . $db_prefix;
   +    Database::addConnectionInfo($key, 'default', $database);
   +    return $key;
   

   out of scope/merge conflict?

2. +++ b/core/tests/Drupal/Tests/Core/Scripts/TestSiteInstallCommandTest.php
   @@ -0,0 +1,51 @@
   +      'non-existing-file' => ['The file this-class-does-not-exist does not exist.', 'this-class-does-not-exist'],
   +      'no-class' => ['The file core/tests/fixtures/empty_file.php.module does not contain a class', 'core/tests/fixtures/empty_file.php.module'],
   +      'non-setup-class' => ["The class $class contained in $file needs to implement \\$interface", $file],
   

   phpcs nit: these should be wrapped because > 80

Explains why I can't test core with phpunit.

Coming back to this, why isn't it just moving the test into the kernel namespace where the database requirement is explicit? No unit tests should require a database even if one exists. That's the core of the test suite separation.

Another "core of the test separation" is that kerneltest etc... install Drupal for you. Using a unit test allows us to prove that this CLI command has 0 dependency on a Drupal-ized test environment.

not convert it to a kernel test, move it to the kernel namespace. i.e. put it in the kernel test suite.

Not sure what's going on but I've run out of ideas for debugging this. Surely something archaic.

Locally I can run the tests in the patch just fine. Tried with ubuntu's php, official php containers, and Lando's containers and its fine. On a server I use for automated patch testing though I consistently get the following error for both php 7.2 and php 5.6. Same command that's roughly just php ./vendor/bin/phpunit -c ./core --testsuite unit

1) Drupal\Tests\Core\Scripts\TestSiteInstallCommandTest::testSetupFile with data set "non-setup-class" ('The class Drupal\Test\Fixture...erface', 'vfs://test/FixtureSetupFileClass.php')
Failed asserting that exception message 'The file vfs://test/FixtureSetupFileClass.php does not exist.' contains 'The class Drupal\Test\FixtureSetupFileClass contained in vfs://test/FixtureSetupFileClass.php needs to implement \Drupal\TestSite\TestSetupInterface'.

going to NW this since its failing for me. Going to see if I can isolate what is special about the environment its failing in but any ideas would be appreciated.

Spent a lot of time going down rabbit holes thinking it might be something directory related because jenkins does some weird directory names which lead me to figure out #2979150: vfs: directory created during unit tests finally but it wasn't. It was PhpUnit 6 causing this issue, seems providers run different and the VFS trick wasn't working.

Split the VFS test to a separate test.

bah, accidentally included the phpunit 6 composer change in that patch. sorry.

RTBC based on #50, I don't think the problem with the misconfigured env will be a big problem, as that will make other tests fail as well.

+++ b/core/tests/Drupal/Tests/Core/Scripts/TestSiteApplicationTest.php
@@ -208,13 +168,16 @@ public function testInstallInDifferentLanguage() {
+    if (!getenv('SIMPLETEST_BASE_URL')) {
+      $this->markTestSkipped('This test requires the environment variable SIMPLETEST_BASE_URL to set. See core/phpunit.xml.dist.');
+    }

@@ -232,10 +195,11 @@ public function testInstallInDifferentLanguage() {
+    $this->assertEquals([], Database::getConnection('default', $this->addTestDatabase($db_prefix))->schema()->findTables('%'));
     $this->assertCount(0, Database::getConnection('default', $this->addTestDatabase($db_prefix))->schema()->findTables('%'));

@@ -262,6 +226,9 @@ public function testTearDownDbPrefixValidation() {
+   * @throws \SkippedTestError
+   *   Thrown if SIMPLETEST_DB environmental variable is not set.

Now everything is in setUp() how about we remove the other places we skip. Less stuff to maintain. Also I think this patch would be simpler just to add the new setUp().

For me there's way too many different unrelated things going on in the same patch.

• Splitting tests out.
• Moving stuff to VFS
• Adding new assertions - that sure make sense but should replace the $this->assertCount()

A good point. Also, we don't currently skip l tests that require SIMPLETEST_X, we fail. That's something I've been trying to fix for a long long time now. #2714555: Regression in SIMPLETEST environment skipping

Why aren't we just moving this to the functional test suite that requires a database connection? This is a functional test after all.

maybe? simplest solution for now.

derp... have to change the namespace. Wish the Test Bot error was better when the class doesn't match the file...

Took another look at this and I think the failures are just that this test is broken.

Specifically there is this code at the top of the test:

    $simpletest_path = $this->root . DIRECTORY_SEPARATOR . 'sites' . DIRECTORY_SEPARATOR . 'simpletest';
    if (!is_writable($simpletest_path)) {
      $this->markTestSkipped("Requires the directory $simpletest_path to exist and be writable");
    }

And we see testbot creating and granting permissions to the simpletest directory after unit tests and moving into kernel tests.

14:48:52 ---------------- Finished run_tests.phpunit in 92.005 seconds ---------------- 
14:48:52 ----------------   Starting run_tests.kernel   ----------------
14:48:52 Directory created at /var/lib/drupalci/workspace/jenkins-php-7.2-apache_mysql5.5-3824/ancillary/run_tests.kernel
14:48:52 mkdir -p /var/www/html/sites/simpletest/xml
14:48:52 chown -fR www-data:www-data /var/www/html/..

To confirm this I setup all the SIMPLETEST requirements locally and ran the test without the patch and it failed exactly the same way.

Even if that would be a better place, the test can run just fine in the kernel suite as demonstrated by the failures actually catching the broken test. There's no reason to block this imho.

Maybe add getDatabaseStub() to \Drupal\Tests\UnitTestCase (there's already few stub methods)
In related issue this particular test is not covered

Blocks #2613044: Requests are pushed onto the request stack twice, popped once

#70 no longer applies to 9.1.x.

Working on it.

It could use another re-roll after #3172537: Passing a command as string when creating a "Symfony\Component\Process\Process" instance is deprecated since Symfony 4.2, pass it as an array of its arguments instead, or use the "Process::fromShellCommandline()" constructor if you need features provided

@andypost Thanks for the info. Will re-roll after that.

Needs work for reroll as #3172537: Passing a command as string when creating a "Symfony\Component\Process\Process" instance is deprecated since Symfony 4.2, pass it as an array of its arguments instead, or use the "Process::fromShellCommandline()" constructor if you need features provided just commited

Rerolled the patch for 9.1.x
Please review

Strange, can't reproduce this same fail locally - the error output is the same as the expected message. However the output seems to depend on the window width, as the second assertion fails for me - the formatting breaks the message in a different place.

Fixing the coding standards issue, let's see what happens here.

Rerolled.

Locally (ddev) I get the error reported in #82.

There are some PHPCBF errors with the patch that need to be fixed. For that only is the issue for a novice.

I have been forgetting to run commit-code-check lately,

I am working on it

PFA files,

The patch just deletes a test now, this isn't right.

Oh...thanks..Let me re-work on this

This still isn't right, I think we need to start again from #88.

Kindly ignore my previous patches.
I believe this time it will be helpful.

I am removing the Novice tag from this issue because I do not think this is a good Novice issue anymore based on @daffie's comments.

Also interestingly, this might be why I ran into issues trying to write tests for #2894476: [PP-1] Provide commands which are helpful for core development.

Just a reroll. I started from #88 because of comment #95 and the latest patch, #96, is adding an interface without explanation.

The failing test is testInstallWithNonSetupClass which I take to mean a test script without a setup method. So, I made such a class and used that in the test. That avoided the error message about The class SebastianBergmann\Version and gave instead one about the new test class. But the assertions were failing because the command output gets outputs with a small width. To get around that I changed \Drupal\BuildTests\Framework\BuildTestBase::executeCommand to use a width of PHP_INT_MAX (as in the original test) and now the test passes.

This test failed randomly on PHP 8.3 in #3375693: Fix deprecated assert_options() function usage for PHP 8.3

Ref https://dispatcher.drupalci.org/job/drupal_patches/195199/testReport/jun...

This looks really great!

Ony tiny nit I stumbled over whilst doing a review:

--- a/core/tests/Drupal/Tests/Scripts/TestSiteApplicationTest.php
+++ b/core/tests/Drupal/BuildTests/Scripts/TestSiteApplicationTest.php
@@ -22,11 +19,13 @@
  * @runTestsInSeparateProcesses
  * @preserveGlobalState disabled
  *
+ * @requires externalCommand composer

[snipped]

@@ -49,35 +58,40 @@ protected function setUp(): void {
    * @coversNothing
    */
   public function testInstallWithNonExistingFile() {
+    $this->copyCodebase();
+    $this->executeCommand('composer install');
 
     // Create a connection to the DB configured in SIMPLETEST_DB.
     $connection = Database::getConnection('default', $this->addTestDatabase(''));
     $table_count = count($connection->schema()->findTables('%'));
 
     $command_line = $this->php . ' core/scripts/test-site.php install --setup-file "this-class-does-not-exist" --db-url "' . getenv('SIMPLETEST_DB') . '"';
-    $process = Process::fromShellCommandline($command_line, $this->root);
-    $process->run();
+    $this->executeCommand($command_line);
+
+    $this->assertErrorOutputContains('The file this-class-does-not-exist does not exist.');
+    $this->assertCommandExitCode(1);
 
-    $this->assertStringContainsString('The file this-class-does-not-exist does not exist.', $process->getErrorOutput());
-    $this->assertSame(1, $process->getExitCode());
     $this->assertCount($table_count, $connection->schema()->findTables('%'), 'No additional tables created in the database');
   }
 
   /**
+   * @requires externalCommand composer

We removed the use of @requires externalCommand in #3362083: '@requires externalCommand' is not parsed in PHPUnit 10 since it will be dropped in PHPUnit 10.

By the looks of it the two occurrences of this can be safely removed from the patch.

@Spokje, thanks!

I have removed the two lines per #107. I also sorted the 'use' statements.

Restore drupalci

Thanks @quietone, for me the changes look good now, let's see what Yer Fellow Core Commiters think, RTBC.

+++ b/core/tests/Drupal/BuildTests/Scripts/TestSiteApplicationTest.php
@@ -237,32 +237,28 @@ public function testInstallInDifferentLanguage() {
+    $this->markTestIncomplete('Fix this test as part of https://www.drupal.org/project/drupal/issues/2949229');

If this test is a false positive due to #2949229: SQLite findTables Returns Empty Array on External DB....

+++ b/core/tests/Drupal/BuildTests/Scripts/TestSiteApplicationTest.php
@@ -49,17 +56,19 @@ protected function setUp(): void {
     $this->assertCount($table_count, $connection->schema()->findTables('%'), 'No additional tables created in the database');

...don't other tests here that assert the number of tables have the same problem?

If this is SQLite only should we just skip these all these tests on SQLite?

Here is the result of my research.

First, in this issue findTables is used successfully.

+++ b/core/tests/Drupal/BuildTests/Scripts/TestSiteApplicationTest.php
@@ -85,52 +96,52 @@ public function testInstallWithFileWithNoClass() {
     $this->assertGreaterThan(0, count(Database::getConnection('default', $key)->schema()->findTables('%')));

So skipping issues based on that is not right.

The skip in testUserLogin was first added in #2878269: Modify TestDiscovery so the testbot runs all the tests, where it pointed to this issue.

public function testUserLogin() {
     +    $this->markTestIncomplete('Fix this test in https://www.drupal.org/project/drupal/issues/2962157.');

That was changed in #70 changed it to point to #2949229: SQLite findTables Returns Empty Array on External DB.. But that testUserLogin does not use findTables, so that is the wrong message. There are two todos later in the test which explain that testing user login can't be done successfully. And local testing confirms that the test fails when testing the user login.

Therefore, I have made a new issue for fixing the userLogin test and updated the markTestIncomplete message accordingly.

Random fail? in QuickStartTest, requeued.

Tests are all green. was previously RTBC in 110 and the feedback from 111 appears to be addressed from what I can tell. Going to remark it.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rerolled patch #112, as it no longer applies. Removed needs-roll tag. please review

This is making things harder for us on gitlab CI