TestSiteApplicationTest requires a database despite being a unit test

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

The last submitted patch, 103: 2962157-103.patch, failed testing. View results →

The failing test is testInstallWithNonSetupClass which I take that 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 📌 Fix deprecated assert_options() function usage for PHP 8.3 Needs review

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 🐛 '@requires externalCommand' is not parsed in PHPUnit 10 Fixed 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?

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 🐛 TestSiteApplicationTest requires a database despite being a unit test Needs work 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.

The last submitted patch, 112: 2962157-112.patch, failed testing. View results →

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

The last submitted patch, 118: 2962157-118.patch, failed testing. View results →

This is making things harder for us on gitlab CI