UpdatePathTestBase fails to re-initialize the test site (rebuild container, clear caches) after running the database updates

I'm wondering whether we could get \Drupal\Core\DrupalKernel::getModulesParameter to read the configuration instead. Would this solve the problem already?

This looks good to me and it fixes a problem we have with an update_hook in Thunder where we install the scheduler_content_moderation_integration module. That module adds new fields to node by hook_entity_base_field_info().

+++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
@@ -372,10 +372,25 @@ protected function runUpdates() {
+      foreach (array_keys($this->config('core.extension')->get('module')) as $module) {
+        if (!$module_handler->moduleExists($module)) {

could we do array_diff(array_keys($this->config('core.extension')->get('module')), array_keys($module_handler->getModuleList())) instead here to fetch the new modules in one pass? We'd still have to call ->addModule for each, but it would save a lot of moduleExists calls

I have a similar problem, but in my case, it's when a module is uninstalled. I guess that proposal from #12 by @larowlan would cover that case too.

I have checked patch from #25 with Thunder tests because we are using UpdatePathTestBase and it works correctly. With the patch, we don't have false fails anymore. I have also checked implementation and it looks good.

+++ b/core/modules/field/tests/src/Functional/Update/FieldUpdateTest.php
@@ -192,6 +192,8 @@ public function testFieldUpdate8500() {
+    $this->deletedFieldsRepository = $this->container->get('entity_field.deleted_fields_repository');

Would it not be better to just use \Drupal::service() directly each time and not assign this to a property?

I have checked the changes. It looks good to me.

Very nice, this fixes the test failure from #2725523-43: Add a revision_parent field to revisionable entities :)

Does this new approach mean that we can revert the changes to FieldUpdateTest?

I think we need a new title as well.

Just some cosmetic fixes.

Wrote a CR for this: https://www.drupal.org/node/3032731, and I think the patch is ready to go.

+++ b/core/modules/field/tests/src/Functional/Update/FieldUpdateTest.php
@@ -193,16 +185,19 @@ public function testFieldUpdate8500() {
 
+    // Refresh the service as running updates can rebuild the container.
+    $deleted_fields_repository = \Drupal::service('entity_field.deleted_fields_repository');

nitpick: from what I see, this isn't actually "refreshing" as the variable is set and used the first time here.

Fixed #52 and reverted some test cleanups so the patch can pass on 8.6.x too.

Also updated the CR to mention a change required for test-only configurable fields after #2976035: Entity type CRUD operations must use the last installed entity type and field storage definitions , as we're doing in core in #3031086: Configurable fields that are created only for update path tests should be defined with custom_storage = TRUE.

This is currently a blocker for the Workflow Initiative, specifically for #2725523: Add a revision_parent field to revisionable entities.

+++ b/core/modules/responsive_image/tests/src/Functional/Update/ResponsiveImageUpdateTest.php
@@ -65,6 +65,11 @@ public function testPostUpdateDependency() {
 
+    // This test incorrectly installs the responsive image module so we can't
+    // properly check entity definition updates. Skip failed update checking
+    // and rely on the assertions here.
+    $this->checkFailedUpdates = FALSE;
+

Does this mean the test is deliberately incorrectly installing the module, or is it pointing to a bug in the test? If the latter, is it not possible to fix the test here?

We can fix the test here :)

1. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
   @@ -321,9 +321,38 @@ protected function runUpdates() {
   +        // Note that resetAll() does not reset the kernel module list so we
   +        // have to do that manually.
   +        $this->kernel->updateModules($module_handler_list, $module_handler_list);
   

   note to self: only the array keys of the first argument matter here, so it is fine to pass the same argument twice.

2. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBaseTest.php
   @@ -70,6 +73,42 @@ public function testDatabaseLoaded() {
   +    // Set a value in the cache to prove caches are cleared.
   

   because we're calling ::updateModules on the container, should we test that the 'container.namespaces' container parameter before and after to ensure we have test coverage for that too?

3. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBaseTest.php
   @@ -70,6 +73,42 @@ public function testDatabaseLoaded() {
   +    \Drupal::state()->set('update_test_schema_version', 8001);
   

   can you elaborate on why this is needed?

Thanks @larowlan for the review!

Re #61:

2. Good point, added assertions for 'container.namespaces' as well.

3. That's how the update path test base test works, it reads a schema version from state to figure out which test update function to run :) But while looking into it, I found that the new code was piggybacking on the existing update_test_schema_update_8001() update function, and it would be cleaner to write a separate one for this test.

That's how the update path test base test works, it reads a schema version from state to figure out which test update function to run :) But while looking into it

the interdiff at #62makes that clear, I wasn't familiar with that one - thanks.

The patch from #62 applies cleanly to 8.6.x with a 3way merge, but here's a separate patch if it's easier to commit like this :)

After a quick discussion with @alexpott on the 8.6.x patch, it's worth pointing out that it uses the update_test_schema_update_8003() function name even though update_test_schema_update_8002() doesn't exist (in 8.6.x), but that doesn't matter because which update function runs is controlled by the test via state :)

it's worth pointing out that it uses the update_test_schema_update_8003() function name even though update_test_schema_update_8002() doesn't exist (in 8.6.x)

Now we have two separate patches, would it prevent confusion if used 8003 in 8.7 and 8002 in 8.6?

Agreed with @alexpott, I think it's less confusing to have the same update function number in both 8.7.x and 8.6.x.

Committed f4ef94a and pushed to 8.7.x. Thanks!
c/p as 6cb23d6823 and pushed to 8.6.x