Change static queries to dynamic queries in core/tests/Drupal

Personally, absolutely +1 on the motivation here, but please anyone working on this be aware this was attempted in #2994904: Convert query('SELECT ... FROM {xxx}') to select('xxx')->... in tests, and finally rejected. Wish you better luck :)

Unassigning from myself because i am stuck in other issues.

I have replaced the static queries with dynamic queries from core/tests/Drupal/FunctionalTests and its sub-folder, core/tests/Drupal/KernelTests and its sub-folders. Please review.

Hi @siddhant.bhosale thanks for patch
Patch looks good for me and cleanly applied.

  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 10632  100 10632    0     0  20546      0 --:--:-- --:--:-- --:--:-- 20525
Checking patch core/tests/Drupal/KernelTests/Core/Command/DbDumpTest.php...
Checking patch core/tests/Drupal/KernelTests/Core/Entity/EntityApiTest.php...
Checking patch core/tests/Drupal/KernelTests/Core/Entity/RevisionableContentEntityBaseTest.php...
Checking patch core/tests/Drupal/KernelTests/Core/KeyValueStore/GarbageCollectionTest.php...
Checking patch core/tests/Drupal/KernelTests/Core/Routing/MatcherDumperTest.php...
Checking patch core/tests/Drupal/KernelTests/KernelTestBaseTest.php...
Applied patch core/tests/Drupal/KernelTests/Core/Command/DbDumpTest.php cleanly.
Applied patch core/tests/Drupal/KernelTests/Core/Entity/EntityApiTest.php cleanly.
Applied patch core/tests/Drupal/KernelTests/Core/Entity/RevisionableContentEntityBaseTest.php cleanly.
Applied patch core/tests/Drupal/KernelTests/Core/KeyValueStore/GarbageCollectionTest.php cleanly.
Applied patch core/tests/Drupal/KernelTests/Core/Routing/MatcherDumperTest.php cleanly.
Applied patch core/tests/Drupal/KernelTests/KernelTestBaseTest.php cleanly.

@adityasingh Thanks for reviewing the patch. Can you please change the status to RTBC ?

Working on this.

Here I have tried to address comment #12.

It still needs work to remove the static query from:
-core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBaseTest.php

I am working on this

Kindly review a patch , covered points #16.1 , #16.2 and #16.4 as suggested by @daffie. #16.3 is still needs work.

Kindly find proper interdiff , last one was diff of diff kindly avoid that.

I don't know somehow i have missed this , changing now 'tr.name' to just 'name'.

Hi @Hardik_Patel_12, I was working on the same issue as you can notice on #15! However, I have checked your patch as well and this is looking fine to me. I am adding the missing point #16.3 and moving this it NR.

@rajandro , this was my bad but it was unintentional. Will make sure in future that if someone is working on some issue then not to pick that issue.

Committed 65656ac660 and pushed to 9.1.x. Thanks!

I'll backport to 9.0.x and maybe 8.9.x if the tests are green to keep tests aligned.

Re #3 I think the statement in the issue summary that using query strings like this makes it harder for contrib db drivers it true so yeah let's do this now and credit people who worked on #2994904: Convert query('SELECT ... FROM {xxx}') to select('xxx')->... in tests

+++ b/core/tests/Drupal/KernelTests/Core/KeyValueStore/GarbageCollectionTest.php
@@ -46,12 +46,12 @@ public function testGarbageCollection() {
         ->keys([
-            'name' => 'key_' . $i,
-            'collection' => $collection,
-          ])
+          'name' => 'key_' . $i,
+          'collection' => $collection,
+        ])
         ->fields([
-            'expire' => REQUEST_TIME - 1,
-          ])
+          'expire' => REQUEST_TIME - 1,
+        ])

+++ b/core/tests/Drupal/KernelTests/Core/Routing/MatcherDumperTest.php
@@ -33,6 +33,9 @@ class MatcherDumperTest extends KernelTestBase {
+  /**
+   * {@inheritdoc}
+   */

+++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
@@ -224,7 +224,7 @@ public function testBootKernel() {
-    // The 'Australia/Sydney' time zone is set in core/tests/bootstrap.php
+    // The 'Australia/Sydney' time zone is set in core/tests/bootstrap.php.

@@ -323,7 +323,7 @@ public function testProfileModules() {
    *
    * @group legacy
    * @expectedDeprecation AssertLegacyTrait::assertIdenticalObject() is deprecated in drupal:8.0.0 and is removed from drupal:10.0.0. Use $this->assertEquals() instead. See https://www.drupal.org/node/3129738
-    */
+   */

Out-of-scope changes. While these changes are correct they are out-of-scope for changing static queries to dynamic queries. I've reverted them from the committed patch. Please don't make such changes in future patches it increases the amount a reviewer has to review.

I had to revert this issue because it broke sqlite testing...

14) Drupal\KernelTests\KernelTestBaseTest::testAssertIdenticalObject
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General
error: 1 no such table: test75591109.test75591109sqlite_master: SELECT
"sql_m"."name" AS "name"
FROM
{test75591109sqlite_master} "sql_m"
WHERE ("type" = :db_condition_placeholder_0) AND ("name" LIKE
:db_condition_placeholder_1 ESCAPE '\') AND ("name" NOT LIKE
:db_condition_placeholder_2 ESCAPE '\'); Array
(
    [:db_condition_placeholder_0] => table
    [:db_condition_placeholder_1] => %
    [:db_condition_placeholder_2] => sqlite_%
)


/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/Connection.php:857
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php:359
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/Connection.php:821
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/Query/Select.php:510
/Users/alex/dev/sites/drupal8alt.dev/core/tests/Drupal/KernelTests/KernelTestBaseTest.php:304
/Users/alex/dev/sites/drupal8alt.dev/vendor/phpunit/phpunit/src/Framework/TestResult.php:691

This should make the following change to \Drupal\KernelTests\KernelTestBaseTest::tearDown

    if ($connection->databaseType() === 'sqlite') {
      $result = $connection->query("SELECT name FROM " . $this->databasePrefix . ".sqlite_master WHERE type = :type AND name LIKE :table_name AND name NOT LIKE :pattern", [
        ':type' => 'table',
        ':table_name' => '%',
        ':pattern' => 'sqlite_%',
      ])->fetchAllKeyed(0, 0);

      $this->assertTrue(empty($result), 'All test tables have been removed.');
    }
    else {
      $tables = $connection->schema()->findTables($this->databasePrefix . '%');
      $this->assertTrue(empty($tables), 'All test tables have been removed.');
    }

i.e. swap the if around. We can use a static query here because we're hardcoding the db type. And the if this way around makes it more obvious.

Would be great to elaborate in IS why change to alterable query makes things less fragile.
In my experience dynamic query (especially to entity tables) are evil because some unpredictable alters are fire and every query needs to skip access when running tests.

I can't address #41, but I'll work on #37

I've made the changes suggested by @alexpott in #37.

Committed fe38a9c and pushed to 9.1.x. Thanks!