DependencySerializationTrait depends on removed __PHPUNIT_BOOTSTRAP global

I assume this also triggers kernel test failures but I haven't gotten into testing Kernel tests yet.

Interestingly, the Unit test suite failure in ViewUIObjectTest::testSerialization exposes a test that isn't actually testing what it thinks it is. The test is trying to test that the object can safely serialize and unserialize. But because of this hack in DependencySerializationTrait, the unserialize component of the test "succeed" with a broken object because the hack allows the code to continue and silently unserialize without the container to populate the code.

This exposes that we can never really trust any tests of this code because they'll always behave differently in a test environment from a real Drupal site which is very bad.

This and the fact that we're embedding testing logic in production code which has its own code smell for performance and complexity reasons leads me to wonder if the entire hack should be removed in favor of addressing the problem somewhere else. Don't know if that's possible but it seems like it would likely be a better solution.

Maybe we should try removing this hack entirely on PHPUnit 9 and seeing how badly things break?

Wow! Not nearly as bad as I expected with all the dire warnings in the trait. Only the known views test is failing. Let me move that over to a kernel test and we'll at least have a green merge request.

Needs rebase and understanding why the tests failed

Tests were failing due to missing @group annotation on the newly introduced test.

Wonder if IS could be updated to included proposed solution. Not sure if this counts as API change?

Updated IS

Since the changes should only affect changing I don't think this counts as an "API Change" so captured that in the IS.

Small comments on MR. Probably good to self RTBC after.

done

Believe this is is good now

Difficult to decide whether to backport this one or whether we need a change record. This seems somewhat unlikely to affect contrib or custom tests unless they are specifically checking dependency serialization so decided to backport to 10.3.x to keep things in sync, but not 10.2.x, and that we don't need a change record as it is an edge case for tests only; testing serialization of dependencies should have been done in a kernel test anyway.

Committed and pushed 24f8f3c961 to 11.x and dfa7222ae5 to 10.3.x. Thanks!

longwave → closed merge request !5693

Reverted, after commit this broke DrupalDateTimeTest::testSleep.

$ ddev test core/tests/Drupal/Tests/Core/Datetime/DrupalDateTimeTest.php 
PHPUnit 9.6.15 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Core\Datetime\DrupalDateTimeTest
................E                                                 17 / 17 (100%)

Time: 00:00.116, Memory: 6.00 MB

There was 1 error:

1) Drupal\Tests\Core\Datetime\DrupalDateTimeTest::testSleep
Drupal\Core\DependencyInjection\ContainerNotInitializedException: \Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container.

/var/www/html/drupal/core/lib/Drupal.php:169
/var/www/html/drupal/core/lib/Drupal/Core/DependencyInjection/DependencySerializationTrait.php:75
/var/www/html/drupal/core/tests/Drupal/Tests/Core/Datetime/DrupalDateTimeTest.php:289
/var/www/html/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/var/www/html/drupal/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/var/www/html/drupal/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/var/www/html/drupal/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/var/www/html/drupal/vendor/phpunit/phpunit/src/TextUI/Command.php:97

longwave → reopened merge request !5693

This was added recently in 🐛 DrupalDateTime serialization issue Fixed

Moved DateTimeTest::testSleep from UnitTest to KernelTest.

Tests appear green again so remarking.

Given that I broke HEAD once here, changed my mind on #16. Let's add a short change record that explains that if you were testing dependency serialization in a unit test you will need to switch to a kernel test.

For review: https://www.drupal.org/node/3425462 →

CR reads fine. So this change is for 11.x only correct?

Technically, yes, but we can also make it in 10.3.x if we want, with the minor risk of breaking some contrib or custom code tests - but as there are only two in core that are affected the chances of any others feels quite slim.

Gotcha, thanks for that.

If I have to work this out when a contrib or custom test suddenly fails on me I'm going to be a little bit frustrated. This change will break someone's contrib or custom tests somewhere. In fact I think it's more like than core as some contrib and custom much prefer to wirte unit tests over other types of tests because they are lighter and easier to manage.

An alternate fix would be use the constant PHPUNIT_COMPOSER_INSTALL - we ensure it is set it in core/tests/bootstrap.php and it is still used in PHPUnit 11 - see https://github.com/sebastianbergmann/phpunit/blob/2a6d51d23035bfcd0fe6c4...

So if do the one line change

    // Tests in isolation potentially unserialize in the parent process.
    $phpunit_bootstrap = defined('PHPUNIT_COMPOSER_INSTALL');

Then everything continues to work and you can unit test serialization of objects that use the DependencySerializationTrait

The problem is that we are calling \Drupal::getContainer() when there is no container. But also in the unit test case there are no service IDs to restore in wakeup either, so we can just skip calling getContainer() in that case. MR!6914 does this and passes locally without changing any tests.

So according to #2553655: Convert ViewKernelTestBase to use KernelTestBaseTNG → we added this code largely to prevent deserialization errors in PHPUnit on failure.

I've tested @longwave's new approach with \Drupal\Tests\user\Kernel\Condition\UserRoleConditionTest::testConditions and made it fail. We don't get serialisations issues anymore! Yay! We think this is due to changes in PHPUnit include the golden oldie https://github.com/sebastianbergmann/phpunit/issues/4983

alexpott → changed the visibility of the branch 3406024-dependencyserializationtrait-depends-on to hidden.

Updated the IS and unpublished the change record as we no longer need it.

catch → closed merge request !6914

Let's try again. Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

At some point this changed from using \Drupal::container to Drupal::service() in a loop. Could that potentially add a bunch of extra function calls to a wakeup?

Oh and it removed all the other changes. So, I think the failure was probably important. Wasn't it saying "hey, you're not testing what you think you're testing. There's not actually a container available."

hmm but isn't that why it was moved into the loop - so that it only runs if there are actual service IDs to operate on, so it doesn't run when there's no container.

But... we could probably check if _serviceIds has anything in/whether there's a container, before getting the container to achieve the same thing.

That makes sense as an optimization.

I guess I'm worried that the tests we where changing where explicitly testing container aware serialization without a container. That's kinda a red flag to me but maybe we can't force better testing with logic outside the test suite.

> \Drupal::container to Drupal::service() in a loop. Could that potentially add a bunch of extra function calls to a wakeup?

To me this is a micro-optimisation and not worth worrying about; we are adding one extra function call for each service that is being reinitialised on wakeup.

We worry about function calls in a ton of other circumstances that might be hot code paths. Not sure why this would be different so follow up MR posted with the approach Catch mentioned.

As far as the tests, they are clearly not testing what they think they are. The only reason to test serialization on them is to assert they're correctly interacting with dependency serialization which they're bypassing so there's definitely a bug. If we're not fixing it here, then we need a follow up.

I wish we could help surface this sort of trap to developers because its obviously easy to fall in. But like I said, I guess we can't build test suite logic into things outside the test suite. That sort of code is what we where removing from this trait to start with its just unfortunate.

Looks good to me now!

Committed and pushed 1707169f2b to 11.x and a20d399646 to 10.3.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.