Drupal 8.8/8.9/9.0 update test coverage

So we could theoretically install umami on 8.8.x and update it to Drupal 9 and run its tests - that'd give us multilingual content to test against. But OTOH we said we'd never support umami upgrades and reserve the right to break things. So maybe we could install umami, change the theme to another theme, hack the profile.... ugh.. I dunno maybe that'd work.

@catch

- 8.8: This makes sense. I think we can write a test to do that for us and it will generate the new DB dump at the end.
- 8.9: Are we going to use above D8.8 DB dumps for this branch as well? Do we want to do that for all the dumps in the code atm?
- 9.0: In #3087644: Remove Drupal 8 updates up to and including 88**, I'm adding a new D9 DB dump to make sure upgrade path tests keep working in D9. We can bring those parts over here unless we want to keep those there and forward port the D8.8 dumps to D9?

Going to have a crack at this.

We have following DB dumps.

ll core/modules/system/tests/fixtures/update/*.gz
core/modules/system/tests/fixtures/update/drupal-8.0.0-rc1-filled.standard.entity_test_update_mul.php.gz
core/modules/system/tests/fixtures/update/drupal-8.0.0-rc1-filled.standard.entity_test_update_mul_rev.php.gz
core/modules/system/tests/fixtures/update/drupal-8.0.0-rc1-filled.standard.entity_test_update.php.gz
core/modules/system/tests/fixtures/update/drupal-8.2.0.bare.standard_with_entity_test_revlog_enabled.php.gz
core/modules/system/tests/fixtures/update/drupal-8.4.0.bare.standard.php.gz
core/modules/system/tests/fixtures/update/drupal-8.6.0.bare.testing.php.gz
core/modules/system/tests/fixtures/update/drupal-8.6.0-minimal-with-warm-caches.sql.gz
core/modules/system/tests/fixtures/update/drupal-8.bare.standard.php.gz
core/modules/system/tests/fixtures/update/drupal-8.filled.standard.php.gz
core/modules/system/tests/fixtures/update/drupal-8-rc1.bare.standard.php.gz
core/modules/system/tests/fixtures/update/drupal-8-rc1.filled.standard.php.gz

After discussing with @larowlan in going to create following dumps
8.8.0.bare.standard, 8.8.0.filled.standard, 8.8.0.bare.testing, 8.8.0.bare.minimal-with-warm-caches
Please let me know if we that is enough or not enough.

I don't think 8.8.0.bare.minimal-with-warm-caches (or any dump with warm caches for that matter) is relevant anymore after #3055443: Switch to a null backend for all caches for running the database updates.

Thanks, I'll remove it.

Here is the patch which adds three more DB dumps 8.8.0.bare.standard, 8.8.0.filled.standard, and 8.8.0.bare.testing after running updates on exiting ones.

I hacked \Drupal\FunctionalTests\Update\UpdatePathTestBaseTest and \Drupal\Tests\system\Functional\Update\UpdatePathTestBaseFilledTest to use new core/modules/system/tests/fixtures/update/drupal-8.8.0.bare.standard.php.gz and core/modules/system/tests/fixtures/update/drupal-8.8.0.bare.standard.php.gz and they are passing now. Should we add new 8.8 only tests?

I have also added core/modules/system/tests/fixtures/update/drupal-8.8.0.bare.testing.php.gz but I'm not sure what type of test is best suited for this.

Do not test patch is contains all the code changes.
Also, uploading update-db-dump.txt which I used to generate the new dumps for the old dump.

Left the existing tests alone and copied them to new one also added test for testing db dump.

I'm not sure we can add a lot more coverage here because we have no specific updates to test yet,

We are checking bare minimum here in my opinion which should be fine for the scope of this issue.

it's test coverage for a hypothetical future problem with router entries so might as well leave it in?

Given that #3087644: Remove Drupal 8 updates up to and including 88** is going to remove the old tests I'd say let's keep it.

Is anything left here to address?

I think it is okay to change the dumps that core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBaseTest.php and core/modules/system/tests/src/Functional/Update/UpdatePathTestBaseFilledTest.php use so we don't have to delete them in 9.0.x.

These tests test update functionality not the dumps or specific updates.

Also I don't understand why the patch is adding both core/modules/system/tests/src/Functional/Update/UpdatePathTestBase88BareTest.php and core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase88Test.php - ah I guess this is to test both core/modules/system/tests/fixtures/update/drupal-8.8.0.bare.testing.php.gz and core/modules/system/tests/fixtures/update/drupal-8.8.0.bare.standard.php.gz. I think we shouldn't add core/modules/system/tests/fixtures/update/drupal-8.8.0.bare.testing.php.gz here. We've only used 4 times since it was added. I think testing updates with as many modules and other updates happening is a good thing. Side effects and unexpected results here are common (unfortunately).

What I'm trying to say in #19 is that we should change \Drupal\FunctionalTests\Update\UpdatePathTestBaseTest and \Drupal\Tests\system\Functional\Update\UpdatePathTestBaseFilledTest to use the new 8.8 dumps rather than duplicating them.

Addressed #19 and #20.

We don't need core/modules/system/tests/src/Functional/Update/UpdateSchemaTest.php anymore.

Here's the smallest possible change here. Which is what I think we want in order to preserve BC. So this patch does not change the user 1 account of the filled db. It also doesn't unnecessarily change the update numbers of the test module or remove any other tests.

I've rebuilt the db dumps from the original 8.0.0 dumps because I can't explain how user 1 had their name and password by the process in #11.

My process is this:

1. gunzip the dump (ie. gunzip -c core/modules/system/tests/fixtures/update/drupal-8.bare.standard.php.gz > drupal-8.bare.standard.php
2. drop the entire site db. (ie. mysql -e "drop database drupal8alt; create database drupal8alt;"
3. load the db (ie. drush src drupal-8.bare.standard.php
4. run updates when on 8.8.x (ie. drush updb -y)
5. dump db (ie. php core/scripts/dump-database-d8-mysql.php > drupal-8.8.0.bare.standard.php
6. gzip drupal-8.8.0.bare.standard.php and copy it to the correct location

+++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBaseTest.php
@@ -37,8 +38,8 @@ public function testDatabaseLoaded() {
-    foreach (['user', 'node', 'system', 'update_test_schema'] as $module) {
-      $this->assertEqual(drupal_get_installed_schema_version($module), 8000, new FormattableMarkup('Module @module schema is 8000', ['@module' => $module]));
+    foreach (['user' => 8100, 'node' => 8700, 'system' => 8805, 'update_test_schema' => 8000] as $module => $schema) {
+      $this->assertEqual(drupal_get_installed_schema_version($module), $schema, new FormattableMarkup('Module @module schema is @schema', ['@module' => $module, '@schema' => $schema]));
     }

This change to the test is useful because it proves we're starting from a 8.8.x dump.

As per @amateescu we should drop the old_ tables from entity updates because they are not part of the update.

@catch yep that was my thinking too. Also if there's no change in this module and in the unlikely case some contrib or custom is using this module for very interesting update testing then at least this change won't break their testing.

I've created #3095333: Extend filled database dump with new stable modules and content for them as a follow-up.

+++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBaseTest.php
@@ -112,15 +113,11 @@ public function testUpdateHookN() {
   public function testPathAliasProcessing() {
     // Add a path alias for the '/admin' system path.
-    $database = \Drupal::database();
-    $database->insert('url_alias')
-      ->fields(['source', 'alias', 'langcode'])
-      ->values([
-        'source' => '/admin/structure',
-        'alias' => '/admin-structure-alias',
-        'langcode' => 'und',
-      ])
-      ->execute();
+    PathAlias::create([
+      'path' => '/admin/structure',
+      'alias' => '/admin-structure-alias',
+      'langcode' => 'und',
+    ])->save();
 

Using the entity API before running updates is a problem. It works now, but it will sooner or later break when we e.g. add another field to it.

It's ugly, but we should do this with raw database queries.

Why not a new db script instead?

DB queries it is.

+++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBaseTest.php
@@ -113,14 +114,27 @@ public function testUpdateHookN() {
+    $revision_table_insert = $database->insert('path_alias_revision');
+    $revision_table_insert->fields(['id', 'revision_id', 'path', 'alias', 'langcode', 'status', 'revision_default']);
+    $values = [
+      'id' => 1,
+      'revision_id' => 1,
+      'uuid' => \Drupal::service('uuid'),
+      'path' => '/admin/structure',
+      'alias' => '/admin-structure-alias',
+      'langcode' => 'und',
+      'status' => 1,
+    ];
+    $base_table_insert->values($values);

service('uuid') gives you the uuid service, not a uuid ;)

This is also quite hard to understand IMHO. I don't think you need fields() at all when you provide values like this. Maybe define $values first for the shared ones, and then do $database->insert()->values($values + specific ones)->execute() twice, instead of building the queries in parallel?

Thanks, for the feedback.
\Drupal::service('uuid') was wrong copy paste.

Ignore #37 this patch addresses #36 properly. Interdiff is from #35.

Yeah, that's much more readable, RTBC if it passes. I didn't look into the database dumps, but @alexpott and @jibran have cross-reviewed and compared that *a lot*.

Fixed the last fails.

Now it passed :)

Just for clarity. Here are two DB diffs.

git diff --no-index -- core/modules/system/tests/fixtures/update/drupal-8.bare.standard.php core/modules/system/tests/fixtures/update/drupal-8.8.0.bare.standard.php > bare-db-8-to-8.8.x.diff

git diff --binary --no-index -- core/modules/system/tests/fixtures/update/drupal-8.filled.standard.php core/modules/system/tests/fixtures/update/drupal-8.8.0.filled.standard.php > filled-db-8-to-8.8.x.diff

There's a CR for these changes at: https://www.drupal.org/node/3098322

@catch and I agreed this doesn't need to be mentioned in the release notes for D9.