Exception trace cannot be serialized because of closure

The three patches makes it really easy to see what is happening, thanks for that!

Most of the test results for patch, 3197324-3-error_after.patch, are back to being helpful. Yay! However, the Kernel Migration tests however are not. They are rather brief and give no clue that the error is in the query.

Migrate_drupal_6.Drupal\Tests\content_translation\Kernel\Migrate\d6\MigrateTaxonomyTermTranslationTest
	✗	
Unknown

fail: [run-tests.sh check] Line 0 of :
FATAL Drupal\Tests\content_translation\Kernel\Migrate\d6\MigrateTaxonomyTermTranslationTest: test runner returned a non-zero error code (255).

I don't know, maybe the other array_walk in executeMigration needs to be changed as well? Or is this error for another issue?

Ran into this too. Root cause: PHPUnit's code — see https://github.com/sebastianbergmann/phpunit/issues/4371.

I was seeing

1) Drupal\Tests\pathauto\Kernel\Migrate\d7\MigratePathautoTest::testMultilingualPathautoMigrations
PHPUnit\Framework\Exception: PHP Fatal error:  Uncaught Exception: Serialization of 'Closure' is not allowed in Standard input code:80
Stack trace:
#0 Standard input code(80): serialize(Array)
#1 Standard input code(112): __phpunit_run_isolated_test()
#2 {main}
  thrown in Standard input code on line 80

Fatal error: Uncaught Exception: Serialization of 'Closure' is not allowed in Standard input code on line 80

Exception: Serialization of 'Closure' is not allowed in Standard input code on line 80

with #3 applied, I'm instead seeing

OK (1 test, 0 assertions)PHP Fatal error:  Uncaught ErrorException: The "PHPUnit\TextUI\ResultPrinter" class is considered internal This class is not covered by the backward compatibility promise for PHPUnit. It may change without further notice. You should not use it from "Drupal\Tests\Listeners\HtmlOutputPrinter". in /Users/wim.leers/Work/d8/vendor/symfony/error-handler/DebugClassLoader.php:386
Stack trace:
#0 [internal function]: PHPUnit\Util\PHP\AbstractPhpProcess::PHPUnit\Util\PHP\{closure}(16384, 'The "PHPUnit\\Te...', '/Users/wim.leer...', 386, Array)
#1 /Users/wim.leers/Work/d8/vendor/symfony/error-handler/DebugClassLoader.php(386): trigger_error('The "PHPUnit\\Te...', 16384)
#2 /Users/wim.leers/Work/d8/vendor/symfony/error-handler/DebugClassLoader.php(312): Symfony\Component\ErrorHandler\DebugClassLoader->checkClass('Drupal\\Tests\\Li...')
#3 /Users/wim.leers/Work/d8/vendor/symfony/phpunit-bridge/DeprecationErrorHandler.php(180): Symfony\Component\ErrorHandler\DebugClassLoader::checkClasses()
#4 [internal function]: Symfony\Bridge\PhpUnit\Dep in /Users/wim.leers/Work/d8/vendor/symfony/error-handler/DebugClassLoader.php on line 386

I'm still not sure what the fix should be. Will try to take a look again later this week.

I can confirm this. This is the kind of stack trace you get:

Database.php:371, Drupal\Core\Database\Database::openConnection()
Database.php:169, Drupal\Core\Database\Database::getConnection()
Query.php:96, Drupal\Core\Database\Driver\sqlite\Select->__wakeup()
AbstractPhpProcess.php:272, unserialize()
AbstractPhpProcess.php:272, PHPUnit\Util\PHP\DefaultPhpProcess->processChildResult()
AbstractPhpProcess.php:171, PHPUnit\Util\PHP\DefaultPhpProcess->runTestJob()
TestCase.php:761, Drupal\Tests\pathauto\Kernel\Migrate\d7\MigratePathautoTest->run()
TestSuite.php:597, PHPUnit\Framework\DataProviderTestSuite->run()
TestSuite.php:597, PHPUnit\Framework\TestSuite->run()
TestRunner.php:627, PHPUnit\TextUI\TestRunner->doRun()
Command.php:204, PHPUnit\TextUI\Command->run()
Command.php:163, PHPUnit\TextUI\Command::main()
phpunit:61, require_once()
ide-phpunit.php:224, IDE_PHPUnit_Loader::loadByAutoloader()
ide-phpunit.php:173, IDE_PHPUnit_Loader::init()
ide-phpunit.php:228, {main}()

The root cause of the problem here is that this call stack I've quoted is a PHPUnit call stack. IOW: this is the PHPUnit process.

But \Drupal\Tests\migrate\Kernel\MigrateTestBase::createMigrationConnection() generates a migrate DB connection on the fly. So I bet the PHPUnit process's unserialization simply is failing because that PHP process does not know about this dynamically constructed DB connection. The stack trace for that function call:

MigrateTestBase.php:102, Drupal\Tests\pathauto\Kernel\Migrate\d7\MigratePathautoTest->createMigrationConnection()
MigrateTestBase.php:68, Drupal\Tests\pathauto\Kernel\Migrate\d7\MigratePathautoTest->setUp()
MigrateDrupalTestBase.php:31, Drupal\Tests\pathauto\Kernel\Migrate\d7\MigratePathautoTest->setUp()
MigrateDrupal7TestBase.php:20, Drupal\Tests\pathauto\Kernel\Migrate\d7\MigratePathautoTest->setUp()
MigratePathautoTest.php:95, Drupal\Tests\pathauto\Kernel\Migrate\d7\MigratePathautoTest->setUp()
TestCase.php:1031, Drupal\Tests\pathauto\Kernel\Migrate\d7\MigratePathautoTest->runBare()
TestResult.php:691, PHPUnit\Framework\TestResult->run()
TestCase.php:763, Drupal\Tests\pathauto\Kernel\Migrate\d7\MigratePathautoTest->run()
Standard input code:58, __phpunit_run_isolated_test()
Standard input code:112, {main}()

Dug deeper and it appears to be even worse:

This PHPUnit process does not know about any DB connection.

I think this is happening because \Drupal\KernelTests\KernelTestBase::$runTestInSeparateProcess

  /**
   * {@inheritdoc}
   *
   * Kernel tests are run in separate processes because they allow autoloading
   * of code from extensions. Running the test in a separate process isolates
   * this behavior from other tests. Subclasses should not override this
   * property.
   */
  protected $runTestInSeparateProcess = TRUE;

(That explains __phpunit_run_isolated_test() — I think)

Imho the root issue is that exceptions thrown in migration kernel tests with a SQL connection aren't serializable.

This is a bit different approach than in #12, but I hope you all will love the test 😊.

Self-review:

1. +++ b/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php
   @@ -177,7 +178,12 @@ protected function executeMigration($migration) {
   +    try {
   +      (new MigrateExecutable($this->migration, $this))->import();
   +    }
   +    catch (ExpectationFailedException $exception) {
   +      self::handleException($exception);
   +    }
   

   MigrateExecutable::import() is wrapped into a try catch...

2. +++ b/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php
   @@ -272,4 +278,70 @@ protected function setTestLogger() {
   +  protected static function handleException(\Exception $exception) {
   +    try {
   +      serialize($exception);
   +    }
   

   ... and then MigrateTestBase::handleException tries to serialize the $exception.

3. +++ b/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php
   @@ -272,4 +278,70 @@ protected function setTestLogger() {
   +      if ($exception instanceof ExpectationFailedException) {
   +        throw new ExpectationFailedException(
   +          implode("\n", [
   +            $exception->getMessage(),
   +            $exception->getLine(),
   +            $exception->getTraceAsString(),
   +          ]),
   +          NULL,
   +          $exception->getPrevious()
   +        );
   +      }
   +      else {
   +        throw self::constructSafeException($exception);
   +      }
   

   If serialization fails, then we try to throw a serializable exception, without the actual trace – but we pass the trace as string to the exception's message.

4. +++ b/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php
   @@ -272,4 +278,70 @@ protected function setTestLogger() {
   +    throw $exception;
   

   But if the exception is serializable, then we just re-throw it.

I have a much simpler and cleaner solution

Self-review:

1. +++ b/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php
   @@ -204,8 +204,8 @@ public function display($message, $type = 'status') {
        if ($this->collectMessages) {
          $this->migrateMessages[$type][] = $message;
        }
   -    else {
   -      $this->assertEquals('status', $type, $message);
   +    elseif ($type !== 'status') {
   +      $this->fail($message);
        }
   

   Yes, this seems to be as easy to solve as you see. Using $this->fail() instead of comparing the catched migration message's #type.

2. +++ b/core/modules/migrate/tests/src/Kernel/MigrateTestExceptionIsSerializableTest.php
   @@ -0,0 +1,60 @@
   +    // Unfortunately we cannot use ::expectException and ::expectException
   +    // methods here, because PHPUnit behaves differently if we do so.
   +    try {
   +      $this->executeMigrations(['d7_file']);
   +    }
   +    catch (\Throwable $throwable) {
   +    }
   

   I had to catch \Throwable because \Exception does not apply on \PHPUnit\Framework\ExpectationFailedException.

3. +++ b/core/modules/migrate/tests/src/Kernel/MigrateTestExceptionIsSerializableTest.php
   @@ -0,0 +1,60 @@
   +    try {
   +      serialize($throwable);
   +    }
   +    catch (\Exception $throwable_serialize_exception) {
   +      $this->fail('Exception thrown in a migration kernel test is not serializable.');
   +    }
   

   This is basically the same as in #14.

@Matroskeen, which version?

Fixed a comment in test.

Never touch any files after commit-code-check.sh succeeds.

Nice to see work here. I applied the #25 and then made an error in \Drupal\Tests\migrate_drupal\Kernl\d7\FieldDiscoveryTest::addAllFieldProcessesAltersData, run the test and got the serialization error.

PHPUnit\Framework\Exception: PHP Fatal error: Uncaught LogicException: The database connection is not serializable. This probably means you are serializing an object that has an indirect reference to the database connection. Adjust your code so that is not necessary. Alternatively, look at DependencySerializationTrait as a temporary solution. in /var/www/html/core/lib/Drupal/Core/Database/Connection.php:1933

@quietone, that means that we cannot continue with the simple fix I guess 🙁

Running into this with Commerce Migrate tests (#3228297: Update D7 fixtures using updated db-tools command in 9.3) :-\

In June the following fix was made available: https://github.com/mpyw/phpunit-patch-serializable-comparison

Should we include that as a dev dependency in core?

FWIW the mpyw/phpunit-patch-serializable-comparison library helped solve the problem for Commerce Migrate, with it in place I was able to see that there was a failed test due to a missing dependency.

If it helps, the error referred to in #33 occurred in \Drupal\migrate\MigrateExecutable::import at

    catch (RequirementsException $e) {
      $this->message->display(
        $this->t(
          'Migration @id did not meet the requirements. @message @requirements',
          [
            '@id' => $this->migration->id(),
            '@message' => $e->getMessage(),
            '@requirements' => $e->getRequirementsString(),
          ]
        ),
        'error'
      );

Still happening and still an absolute pain to deal with.

The problem showed up in Metatag in #3252159: Fix tests compatibility with Drupal 9.3.x and again was resolved by using the Composer dependency.

So here are patches for 9.3.x and 8.4.x to add mpyw/phpunit-patch-serializable-comparison as a dev dependency.

Created two patches which will hopefully prove that the patch in #38 will allow a helpful error message to display instead of the about 'closure'. Both patch have the same forced failure in two test files. One patch,3197324-39-fail-closure.patch, has only the changes to the tests and should result in the useless error message. The other patch include the patch from #38 and should result in an error message that is helpful.

The tests are failing as expected. If you search the results 3197324-39-fail.patch for 'closure' it is not present, unlike for the other patch.

I'll try the patch in #38, the next time I get the 'Exception trace cannot be serialized because of closure' error.

I like it, but seeing it adds a new core dependency, I suppose we need to do the Core Dependency Criteria Dance?

The simplest possible solution is what Sebastian Bergman said: don't call assertion in closures.

Yes, he did say that and closed the issue. However, there have been additional comments providing examples of the problem which do not include closures. A later comment in that issue is where mpyw gives a link to a patch they wrote, which is the solution suggested by DamienMcKenna in #38.

Edit: After posting I saw that @matroskeen posted this same fix in #3 (and in #5 on the parent issue). I've tried to read the early part of this issue more closely and I don't really understand why we moved away from the simplest approach. Maybe we can reassess based on the latest comments. Now back to my original comment...

The simplest possible solution is what Sebastian Bergman said: don't call assertion in closures.

With this in mind, I changed the outer array_walk to a foreach and that fixes the issue locally for me. There's no interdiff because this is a new approach. It's super simple and introduces no new dependencies.

Following the steps to reproduce (mussing the source Term source plugin) without this patch, I get

1) Drupal\Tests\taxonomy\Kernel\Migrate\d6\MigrateTaxonomyTermTest::testTaxonomyTerms
PHPUnit\Framework\Exception: PHP Fatal error:  Uncaught Exception: Serialization of 'Closure' is not allowed in Standard input code:80
Stack trace:
#0 Standard input code(80): serialize(Array)
#1 Standard input code(112): __phpunit_run_isolated_test()
#2 {main}
  thrown in Standard input code on line 80
Fatal error: Uncaught Exception: Serialization of 'Closure' is not allowed in Standard input code:80
Stack trace:
#0 Standard input code(80): serialize(Array)
#1 Standard input code(112): __phpunit_run_isolated_test()
#2 {main}
  thrown in Standard input code on line 80

/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:270
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:627
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:656
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:235
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:194
/var/www/html/vendor/phpunit/phpunit/phpunit:92

But after applying my patch I get a flurry of fatal errors, but they are drupal errors you might expect if your source plugin is hosed.

I am not convinced that is sufficient.

With the patch in #45 and a forced failure in \Drupal\Tests\migrate_drupal\Kernel\d7\FieldDiscoveryTest::testAddAllFieldProcessesAlters the error is

 1) Drupal\Tests\migrate_drupal\Kernel\d7\FieldDiscoveryTest::testAddAllFieldProcessesAlters with data set "Field Widget" ('alterFieldWidgetMigration', array(array(array(array('d7_text_default', 'number_default_default', 'taxonomy_term_reference_default', 'image_default', 'link_default', 'entityreference_default', 'options_select', 'options_buttons', 'entity_reference_autocomplete_tags', 'options_select', 'options_buttons', 'entity_reference_autocomplete_tags', 'list_default', 'file_generic', 'file_generic', 'datetime_default', 'datetime_default', 'datetime_timestamp', 'email_default', 'phone_default')))))
PHPUnit\Framework\Exception: PHP Fatal error:  Uncaught AssertionError: The container was serialized. in /var/www/html/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php:160
Stack trace:
#0 /var/www/html/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php(160): assert(false, 'The container w...')
#1 [internal function]: Drupal\Core\DependencyInjection\ContainerBuilder->__sleep()
#2 Standard input code(80): serialize(Array)
#3 Standard input code(112): __phpunit_run_isolated_test()
#4 {main}
  thrown in /var/www/html/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php on line 160

Fatal error: Uncaught AssertionError: The container was serialized. in /var/www/html/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php on line 160

AssertionError: The container was serialized. in /var/www/html/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php on line 160

With the patch in #38 the error is:

1) Drupal\Tests\migrate_drupal\Kernel\d7\FieldDiscoveryTest::testAddAllFieldProcessesAlters with data set "Field Widget" ('alterFieldWidgetMigration', array(array(array(array('d7_text_default', 'number_default_default', 'taxonomy_term_reference_default', 'image_default', 'link_default', 'entityreference_default', 'options_select', 'options_buttons', 'entity_reference_autocomplete_tags', 'options_select', 'options_buttons', 'entity_reference_autocomplete_tags', 'list_default', 'file_generic', 'file_generic', 'datetime_default', 'datetime_default', 'datetime_timestamp', 'email_default', 'phone_default')))))
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
                 'number_default' => 'number_default_default'
                 'taxonomy_term_reference' => 'taxonomy_term_reference_default'
                 'image' => 'image_default'
+                'image_miw' => 'image_image'
                 'link_field' => 'link_default'
                 'entityreference' => 'entityreference_default'
                 'node_reference_select' => 'options_select'

I much prefer the error report using #38. It is showing the exact error I made.

And here is the a patch with the forced test failure.

Thanks, @quietone!

That is an excellent example of where there appears to be no "simple" solution. I thought maybe the culprit was the sort calls within $this->assertSame(sort($expected_process_keys), sort($actual));, which is an unusual pattern, but refactoring that did not change anything.

I'm sold on the need for a robust solution like the dependency being discussed. I'm going slowly back from this issue now...

:)

Ran into this again today.
I went to use drush on 10.0.x and that fails, Drush 11 cannot be composer required/updated after Drupal's bump to Symfony 6, nor does other versions of drush. Migrate Tool isn't compatible either so I went back to just using the migration kernel tests and found myself back here. (Not a great start to the day).

I am raising this to a Major because it is a significant developer-facing bug with no workaround. One could argue the workaround is to use a debugger but that is difficult because the error message doesn't give any clue in finding where the bug is. It is all trial and error.

Uploading a patch for 10.0.x but not running tests.

I faced this recently and spent a good hour or so trying to debug & search for a fix until I landed on this issue. +1 to #48

I'm curious where we are with this one. the patch in #48 doesn't have any description or interdiff, but I'm trying to piece through the work here.

Ran into this again today and came here to get the package name to require.

I am not sure what description you are looking for. The goal here is to add a dev dependency on mpyw/phpunit-patch-serializable-comparison so that we get the useful error messages from migration. So, today I was getting Fatal error: Uncaught Exception: Serialization of 'Closure' i ... instead of helpful RequirementsException message.

The patch in #48 needs a reroll.

Besides that, if we decide to go this way, which I myself think is a good idea, we would need an entry for the new dev-dependency here: https://www.drupal.org/about/core/policies/core-dependency-policies/depe...

I think we need some core committer/release manager/people with Big Brains approval on it first? Otherwise we would advertise an entry for a not yet added dependency?

*drops EUR 0.02*

Sorry, @quietone, somehow I missed the last line in #48 saying this was a 10.x patch. I must not have had my coffee yet.

Thankfully @quietone is a provisional release manager, so if we do need to add that dependency, I'm sure she can work through the necessary approvals.

That said, We really shouldn't actually add this package to core. It's a unmaintained one commit hack to work around a bug in https://github.com/sebastianbergmann/comparator. There is a 4 year old issue open in the upstream here https://github.com/sebastianbergmann/comparator/pull/47. The PR there is outdated, it implements Serializable and overrides the serialize() and unserialize() methods, which I believe throws a deprecation in php8.1. It needs to be updated to just use __serialize and __unserialize introduced in php 7.4. The pr hasn't had any maintainer response, despite the project being maintained. The best solution here is to push for a fix upstream. I've added a comment to that PR, to see if we have any chance. Otherwise, it's going to be tough to implement this override in core in a reasonable way. the mpyw package uses composer to autoload his copy of SebastianBergmann\Comparator\ComparisonFailure first, so the autoloader never gets the real copy, but that loads the file whether it's needed or not, and leaves things prone to incompatibility issues if the upstream changes that file, so it's not really a good solution for core. Short of forking the whole comparator library (also far from ideal, for obvious reasons) I think it's going to be pretty tricky, although perhaps @alexpott or @catch have some other idea that I'm not thinking of.

The best solution is to get this fixed upstream, if we possibly can. The existing PR has no issue associated with it, perhaps we could start with creating an issue and an updated PR and see where we can get.

We did get a commitment from the maintainer to at least review a new patch if we create one. https://github.com/sebastianbergmann/comparator/pull/47#issuecomment-117...

@mikelutz, thanks for moving this along!

I've updated the proposed resolution and remaining tasks based on #53.

Tried to get the ball roling in https://github.com/sebastianbergmann/comparator/pull/106

For the record, I created a solution as part of

https://git.drupalcode.org/project/drupal/-/commit/4cffbd777c827b7fa9b00...

There is a trait that does this:

  protected function onNotSuccessfulTest(\Throwable $t): void {
    $cleaner = new ExceptionCleaner();
    $cleaner->cleanException($t);
    /* @see \PHPUnit\Framework\TestCase::onNotSuccessfulTest() */
    parent::onNotSuccessfulTest($t);
  }

Yes, the correct thing would be for PhpUnit to handle serialization of exception backtraces.
It already does that in most of its exception classes, just not in ComparisonFailure.

And btw, the problem is not so much in throwing or asserting from within a closure, but having a closure as an argument in one of the calls in the trace. For some reason, the closure in the 'function' part is fine.

Being able to use closures more freely in tests opens some interesting and fun possibilities.

I am not opposed to the "mpyw/phpunit-patch-serializable-comparison" as in the patch here.
The exception cleaner could be more powerful in cleaning up other exceptions that are not covered by that package.
But actually in my experiments, other exceptions typically don't cause the same problems.
I think PhpUnit wraps them in its own exception classes before they would get serialized.
ComparisonFailure is special because it is stored in ExpectationFailedException->comparisonFailure.

I ran into this again at #3364108: Configuration schema & required keys. This has cost me dozens of hours of wasted time over the past ~3 years 🙈

So, time to fix it forever.

Before

1) Drupal\Tests\file\Kernel\Migrate\d7\MigrateFileConfigsTest::testFileSettings
PHPUnit\Framework\Exception: PHP Fatal error:  Uncaught LogicException: The database connection is not serializable. This probably means you are serializing an object that has an indirect reference to the database connection. Adjust your code so that is not necessary. Alternatively, look at DependencySerializationTrait as a temporary solution. in /var/www/html/core/lib/Drupal/Core/Database/Connection.php:1895
Stack trace:
#0 [internal function]: Drupal\Core\Database\Connection->__sleep()
#1 Standard input code(84): serialize(Array)
#2 Standard input code(123): __phpunit_run_isolated_test()
#3 {main}
  thrown in /var/www/html/core/lib/Drupal/Core/Database/Connection.php on line 1895
Fatal error: Uncaught LogicException: The database connection is not serializable. This probably means you are serializing an object that has an indirect reference to the database connection. Adjust your code so that is not necessary. Alternatively, look at DependencySerializationTrait as a temporary solution. in /var/www/html/core/lib/Drupal/Core/Database/Connection.php:1895
Stack trace:
#0 [internal function]: Drupal\Core\Database\Connection->__sleep()
#1 Standard input code(84): serialize(Array)
#2 Standard input code(123): __phpunit_run_isolated_test()
#3 {main}
  thrown in /var/www/html/core/lib/Drupal/Core/Database/Connection.php on line 1895

https://www.drupal.org/pift-ci-job/2752576

After

1) Drupal\Tests\file\Kernel\Migrate\d7\MigrateFileConfigsTest::testFileSettings
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 ()
+Array &0 (
+    0 => 'Schema errors for file.settings with the following errors: 0 [] 'filename_sanitization' is a required key. (/var/www/html/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:94)'
+)

— https://www.drupal.org/pift-ci-job/2752966

To my surprise, it also reveals some rather big problems in the existing migration tests:

+        'string' => 'Migration @id did not meet the requirements. @message'
+        'arguments' => Array &1 (
+            '@id' => 'd6_comment_type'
+            '@message' => 'The module comment is not enabled in the source site.'
+        )

🫣

I too would love this fixed!

The before and after results in #57 are for two different tests, are you sure the patch is a fix?

I went and tried this with my goto fail for this and I didn't get any change. :-(. I made an error in the data provider for \Drupal\Tests\migrate_drupal\Kernel\d7\FieldDiscoveryTest::testAddAllFieldProcessesAlters and ran the test with and without #58.

The result before and after is that same:

1) Drupal\Tests\migrate_drupal\Kernel\d7\FieldDiscoveryTest::testAddAllFieldProcessesAlters with data set "Field Formatter" ('alterFieldFormatterMigration', array(array(array(array(array('entity_reference_label', 'entity_reference_label', 'entity_reference_label', 'entity_reference_label', 'entity_reference_label', 'entity_reference_entity_view', 'entity_reference_entity_view'), array('link', 'link', 'link', 'link', 'link', 'link', 'link', 'link', 'link', 'link', 'link_separate'), array('entity_reference_label', 'entity_reference_entity_id', 'entity_reference_entity_view'), array('entity_reference_label', 'entity_reference_label', 'entity_reference_entity_id', 'entity_reference_entity_view', 'entity_reference_label'), array('entity_reference_label', 'entity_reference_label', 'entity_reference_entity_id', 'entity_reference_entity_view', 'entity_reference_label'), array('file_default', 'file_url_plain', 'file_url_plain', 'image', 'image', 'image'), array('datetime_default', 'datetime_time_ago', 'datetime_plain'), array('email_mailto', 'basic_string', 'basic_string', 'basic_string', 'email_mailto', 'basic_string', 'basic_string', 'basic_string'), array('basic_string'), array('string', 'telephone_link'))))))
PHPUnit\Framework\Exception: PHP Fatal error:  Uncaught LogicException: Settings can not be serialized. This probably means you are serializing an object that has an indirect reference to the Settings object. Adjust your code so that is not necessary. in /var/www/html/core/lib/Drupal/Core/Site/Settings.php:86

What test produced the output with 'comment is not enabled in the source site'?

The before and after results in #57 are for two different tests, are you sure the patch is a fix?

I had just mixed up the D6 & D7 MigrateFileConfigsTest, but their results are pretty much identical — still, fixed my comment!

Yes I'm sure 🤓

I went and tried this with my goto fail for this and I didn't get any change. :-(. I made an error in the data provider for \Drupal\Tests\migrate_drupal\Kernel\d7\FieldDiscoveryTest::testAddAllFieldProcessesAlters and ran the test with and without #58.

Right, that must have a different root cause then. The root cause for that test must be somewhere else than in MigrateTestBase's asserting the absence of a migration error message.

#58 only fixes the case of $this->assertEquals('status', $type, $message); causing this to happen.

What test produced the output with 'comment is not enabled in the source site'?

See the test results for #58 😊

@Wim Leers, thanks for working on this!

It was late last night when I looked at this. I thought I should look again.

To my surprise, it also reveals some rather big problems in the existing migration tests:

The failing tests in #58 are tests that are testing for specific errors in the migrate messages. So, to assert that there are no migrate messages immediately after executing a migration and before the assertions in the test is breaking those tests.

+++ b/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php
@@ -198,12 +207,11 @@ protected function executeMigrations(array $ids) {
+    // Collect messages even when $this->collectMessages === FALSE, to prevent

I also am not keen on changing the behavior of collectMessages. One could argue that messages should be collected for each Migrate Kernel test so that assertions can be made on the count or the message text. However, that is a significant amount of work for the approximately 230 Migration Kernel tests. Fortunately, we have the test run in #58 to show that only 3 tests have migrate error messages. And those 3 are supposed to have errors, I think we have good evidence that collecting messages during Kernel tests isn't needed. That is good news!