Exception trace cannot be serialized because of closure

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 📌 [PP-1] Configuration schema & required keys Needs review . This has cost me dozens of hours of wasted time over the past ~3 years 🙈

So, time to fix it forever.

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!