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.

Marked 🐛 Cypress tests include invalid menu config on 11.x HEAD Active as a duplicate of this. See how the default menu block plugin configuration changed. We'll need to port that change here most likely.

https://www.drupal.org/project/drupal/releases/11.2.0 → was created ~7 hours ago while I was sleeping :)

So: soon "next minor" will become "current minor", at which point our CI will break unless we force CI back to considering 11.1 current minor.

wim leers → credited effulgentsia → .

wim leers → credited lauriii → .

Discussed with @effulgentsia and @lauriii. We agreed that XB 1.0.0-beta1 should require 11.2, meaning we'll drop 11.1 support.

That will simplify things that may be different between different minors, such as ✨ Enum vales do not have translatable labels Active .

Paired with @isholgueras to accelerate getting this on track, per #56.

Turns out that @longwave's #38 aka 🐛 Modules that create config cannot be installed alongside modules that depend on that config Active is no longer occurring, because @isholgueras and I removed all the config/install/experience_builder.component.*.yml files and tests pass 🥳

PHPUnit CI job failure is caused by a PHP warning:

    1 test triggered 1 PHP warning:
    
    1) /builds/project/experience_builder/src/ComponentSource/ComponentSourceBase.php:75
    Undefined array key "title"
    
    Triggered by:
    
    * Drupal\Tests\experience_builder\Kernel\Config\JavaScriptComponentValidationTest::testInvalidSlotIdentifiedByConfigSchema
      /builds/project/experience_builder/tests/src/Kernel/Config/JavaScriptComponentValidationTest.php:796

AFAICT that's the only thing that still needs to be resolved? 🤞

Investigating #57.

Figured out #57. ComponentSourceBase::normalizeSlotDefinitions() assumes every slot has a title. And that's actually pretty reasonable to expect. Because it's equally crucial for usability as props having titles. So: made that a proper requirement now 👍

but also … LOL, I was wildly wrong in #57 😇 The "next minor" CI job wasn't running at all; I was looking at the current PHPUnit CI job … 🫣

It crashes very early on:

PHP Fatal error:  Uncaught Error: Interface "Drupal\TestSite\TestSetupInterface" not found in /builds/project/experience_builder/tests/src/TestSite/XBTestSetup.php:31

Just tweaked our GitLab CI to only ever test against 11.2. That provides a clearer target: all the CI jobs we're used to looking at must pass 👍

Crediting @penyaskito for https://git.drupalcode.org/project/experience_builder/-/merge_requests/4...

We're green and have PHPUnit running around the 5 min mark.
https://git.drupalcode.org/project/experience_builder/-/jobs/5771862

larowlan → credited fjgarlin → .

larowlan → credited hestenet → .

4 mins 11 https://git.drupalcode.org/project/experience_builder/-/jobs/5771963 😎

🥳🥳🥳

What a lovely bit of news to wake up to! 😊

First dropping Jack off at daycare, then can’t wait to review and land this one 🤓

Awesome work here, thanks everyone! 🙏

Did a complete review to make sure I 100% understand what this MR is changing and why.

Observations (for future reference for everyone, including myself):

1. The changes to XB's config schema support to match the SDC JSON schema for its "code components" were too broad. In the near future, meta:enum and x-translation-context will be added in 📌 Leap ahead of #3493070 in core: SDC `enum` props should have human-readable labels: use `meta:enum` Active . default may be added in the distant future, in 🐛 DX & authoring experience: for SDC+code components, the example is treated as the default, may not be desirable Active .
2. This actually includes the fix for 🐛 [PP-1] Use JSON schema type for sqlite and remove text workaround Active .
3. Bumped #3530351 to critical at #3530351-4: Decouple image (URI) shape matching from specific image file types/extensions → because of the stuff this MR does to support AVIF (which makes sense to follow the latest & greatest in 11.2 👍).
4. The PHPUnit slowness appears to have been caused by a combination of factors in 11.2 which mean that @larowlan's fix for it (removing ui/node_modules 🤪) is suddenly necessary whereas it been necessary during Drupal 10, 11.0 nor 11.1 🤷‍♂️ Likely culprit: 🐛 drupal_phpunit_find_extension_directories() uses infinite recursion ⇒ more directories = slower tests Needs work , which ironically I opened and did a lot of work on 3.5 years ago 🤣
5. The pretty crazy work-around this MR adds to tests/src/TestSite/XBTestSetup.php will be removed in 📌 Decouple kernel tests from XbTestSetup Active 👍
6. The 🐛 Fatal error when passing NULL to Renderer::render() Active regression in Drupal core 11.2 forced the addition of temporary work-arounds. Issue created to ensure we revert those: 📌 [upstream] Revert work-arounds for 11.2 regression #3524738 Postponed .

See y'all on all other s in the next week 😊