PHPUnit Next Major tests failing

@longwave pointed out that 📌 Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller Active was only added to 11.x. The current just switches our phpunit (next major) to use 11.1.x. Assuming this passes test I suggest we commit this and leave the issue to remove the need for the override

Assigning to @wim leers because he needs to .gitlab-ci.yml changes

📌 CI: update to 1.6.3 of the GitLab CI Template Active also pins to 11.1.x, maybe worth doing that instead as Stylelint is also failing but upgrading the template will eventually unblock that via 📌 Update Stylelint formatter for Stylelint 16 Active

I can reproduce the fail locally with

$ ddev xb-set-core-version 11.x-dev
$ ddev test tests/src/Kernel/Plugin/Field/FieldType/ComponentTreeItemTest.php --filter testInvalidField@valid.*props

1) Drupal\Tests\experience_builder\Kernel\Plugin\Field\FieldType\ComponentTreeItemTest::testInvalidField with data set "valid values using static props" (['{"a548b48d-58a8-4077-aa04-da9...ts"}}}', '{"dynamic-static-card2df":{"h...ue"}}}'], [])
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 []
+Array &0 [
+    'field_xb_demo.0' => Array &1 [
+        0 => 'The array must contain a "tree" key.',
+        1 => 'The array must contain a "props" key.',
+    ],
+]

/var/www/html/web/modules/contrib/experience_builder/tests/src/Kernel/Plugin/Field/FieldType/ComponentTreeItemTest.php:233

At a guess, this might be why:

 Drupal\experience_builder\Plugin\ComponentPluginManager:
    decorates: Drupal\Core\Theme\ComponentPluginManager
    parent: Drupal\Core\Theme\ComponentPluginManager
    arguments: ['@entity_type.manager', '@state']

Might be worth trying with 📌 Install modules with container_rebuild_true set by themselves Active applied.

📌 Install modules with container_rebuild_true set by themselves Active indeed does fix it.

Why is the writing component info to state done as a decorator? Could this not be a separate service that operates on e.g. hook_rebuild()?

📌 Install modules with container_rebuild_true set by themselves Active landed so this should no longer be necessary - @tedbow can you confirm?

We should still do 📌 CI: update to 1.6.3 of the GitLab CI Template Active .

re #12

landed so this should no longer be necessary - @tedbow can you confirm?

re-running tests on 0.x. https://git.drupalcode.org/project/experience_builder/-/pipelines/364038

there is e2e test that also appears to be failing but that is unrelated. see 📌 Remove leftover wait() in empty canvas e2e test Active

Looks like it is failing for different reason https://git.drupalcode.org/project/experience_builder/-/pipelines/364038

I triggered the jobs to re-run on the same pipeline so I am not sure we can see the previous results

Seeing if I can re-run the pipeline here with a new commit

Yep still failing

Spotted this too yesterday, thanks for jumping in! 👍🙏

If 11.1.x passes I would suggest we use that and then leave this issue as critical. Only so many people can probably work on figuring this out and meanwhile it would good if other issues could get committed. Also maybe 11.x will be receiving a lot of commits while Drupalcon Singapore is going on?

+1

I think we should do 📌 CI: update to 1.6.3 of the GitLab CI Template Active which will test against 11.1 and 10.4 and is currently green, then we can enable OPT_IN_TEST_NEXT_MINOR to add 11.2 but that's not critical to complete until closer to the release of 11.2.0.

Oh, even better! Looking 👀

(The "+1" was referring to "let's unblock this ASAP".)

Just reviewed #3480805 in depth, and pleased to say I was able to RTBC it! 🚀 Yay for one temporary work-around avoided, and the correct/long-term solution being ready at the same time! 🥳

📌 CI: update to 1.6.3 of the GitLab CI Template Active is in — is there anything left to do here? 😊

Well, we should now opt in to "next minor" in our CI config and then fix the failure.

👍 — retitling.

longwave → changed the visibility of the branch 3492722-phpunit-next-major to hidden.

Nice, that seems to be uncovering either not previously observed bugs or 11.2.x has changed in some way — because tests are failing with unmet config dependencies! 😬

Also:

Warning: Undefined array key "label" in /builds/project/experience_builder/web/core/lib/Drupal/Core/Field/FormatterBase.php on line 80

… this seems an upstream bug in core?

This is less urgent than many other MRs, so unassigning @longwave.

Thanks to @longwave, closed 📌 CI: also test against next minor: 11.2.x aka Drupal 11.x HEAD Active as a duplicate.

@penyaskito found that FieldTypeUninstallValidatorTest fails in HEAD as well

1) Drupal\Tests\experience_builder\Kernel\FieldTypeUninstallValidatorTest::testUninstall
Drupal\Core\Config\UnmetDependenciesException: Configuration objects provided by <em class="placeholder">xb_test_config_node_article</em> have unmet dependencies: <em class="placeholder">field.field.node.article.field_xb_test (experience_builder.component.sdc.experience_builder.image, experience_builder.component.sdc.experience_builder.my-hero, experience_builder.component.sdc.experience_builder.two_column)</em>

/var/www/html/web/core/lib/Drupal/Core/Config/UnmetDependenciesException.php:100
/var/www/html/web/core/lib/Drupal/Core/Config/ConfigInstaller.php:581
/var/www/html/web/core/lib/Drupal/Core/ProxyClass/Config/ConfigInstaller.php:132
/var/www/html/web/core/lib/Drupal/Core/Extension/ModuleInstaller.php:209
/var/www/html/web/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
/var/www/html/web/modules/contrib/experience_builder/tests/src/Kernel/FieldTypeUninstallValidatorTest.php:60

2) Drupal\Tests\experience_builder\Kernel\FieldTypeUninstallValidatorTest::testUninstallXbFieldMultipleEntityTypes
Drupal\Core\Config\UnmetDependenciesException: Configuration objects provided by <em class="placeholder">xb_test_config_node_article</em> have unmet dependencies: <em class="placeholder">field.field.node.article.field_xb_test (experience_builder.component.sdc.experience_builder.image, experience_builder.component.sdc.experience_builder.my-hero, experience_builder.component.sdc.experience_builder.two_column)</em>

/var/www/html/web/core/lib/Drupal/Core/Config/UnmetDependenciesException.php:100
/var/www/html/web/core/lib/Drupal/Core/Config/ConfigInstaller.php:581
/var/www/html/web/core/lib/Drupal/Core/ProxyClass/Config/ConfigInstaller.php:132
/var/www/html/web/core/lib/Drupal/Core/Extension/ModuleInstaller.php:209
/var/www/html/web/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
/var/www/html/web/modules/contrib/experience_builder/tests/src/Kernel/FieldTypeUninstallValidatorTest.php:103

probably due to 📌 Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller Active

Disclaimer: I usually work with 11.x HEAD, so was surprised I didn't see this before.

The problem is that if installing several modules, after experience_builder is installed no rebuild is happening, so it's not triggering the discovery of sdc components/blocks and its related config creation, and xb_test_config_node_article depends on that.

I thought we might need to flush caches on hook_install, but we are actually doing that already.
That's why the issue only happens in (some) kernel tests.

So we have two options:

1) Include the config in xb_test_config_node_article (meh 😔)
2) Document and workaround it in kernel tests (☺️)

e.g Never use

$this->container->get('module_installer')->install(['experience_builder', 'xb_test_config_node_article', 'sdc_test']);

but instead use

$this->container->get('module_installer')->install(['experience_builder', 'sdc_test']);
$this->container->get('module_installer')->install(['xb_test_config_node_article']);

container_rebuild_required = TRUE in xb.info.yml might be enough. See 📌 Consider defaulting requires_container_rebuild to FALSE Active .

Even better, thanks!

#32 onwards is a core bug in the module installer since 📌 Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller Active that means that modules that depend on XB components cannot be installed at the same time that XB itself is installed. This is because XB creates config objects on the fly, that do not exist until after XB is actually installed, but ::install() does this (simplified):

// Gather full set of modules to be installed
...

// Check the validity of the default configuration. This will throw
// exceptions if the configuration is not valid.
$config_installer->checkConfigurationToInstall('module', $module_list);

// Group modules based on container_rebuild_required flag
...

// Install modules in groups
foreach ($module_groups as $modules) {
  $this->doInstall($modules, $installed_modules, $sync_status);
}

Because we check the config dependencies of the entire set before anything is installed, the XB config entities can never exist by this point.

Locally, moving checkConfigurationToInstall() into the later loop makes the tests pass for me, so I will open a core issue to change this.

Wow, #36 is quite the find! 🤯 Thanks, @longwave!

Wrote all this up and was about to open a new issue, then found that I opened 🐛 Modules that create config cannot be installed alongside modules that depend on that config Active back in January and completely forgot about it!

Alternatively though: XB can ship experience_builder.component.sdc.experience_builder.* as default config to work around this, at least for the XB-provided set of components - given this will be created at install time anyway, why can't we ship with it?

I had the same question as #36 at #33, and catch responded in #34.

given this will be created at install time anyway, why can't we ship with it?

Can't really argue with that.

Thanks to \Drupal\Tests\experience_builder\Functional\DefaultConfigTest it should be no pain at all; our tests would tell us. Or they should. If not, then we would need an explicit test for that.

Per #39, did we try @catch's #34?

Meanwhile: testing #40's assumptions.

Yes I tried #34, it doesn't help because container_rebuild_required is not considered until after checkConfigurationToInstall() is called, which is where the error is triggered - see the pseudocode in #36.

Not sure 30291899 is a valid test because SingleDirectoryComponent::updateConfigEntity() only updates prop_field_definitions, not labels - if we don't have a child of 🌱 [META] Production-ready ComponentSource plugins Active to deal with this we need one.

All green! Unsure if we want to make next minor fail CI or if we should just keep an eye on it manually.

#43++, tagging.

#44: how about we mark this CI job to allow failures? Then we can at least see where we're at. Manually triggered on MRs, automatically on commit?

PHPUnit (next minor) already allows failures. Pushed a change to disable _FOR_EVERY_MR_COMMIT for the next minor matrix so we can run them manually.

Opened 📌 Handle update and delete of SingleDirectoryComponent `Component`s, plus missing config dependencies Active as a sister issue of 📌 Handle update and delete of Block component config entities Active .

I wanted to merge this, but:

We want only 1 of the 4 for the next minor, too.