Problem/Motivation

The regressions that @Berdir reported at #3400868: [PP-2] to_email and similar config keys have schema type email but allow other things due to changes in Drupal core 10.2.x should NOT be happening.

Let's make it easy to see just how big the impact is by always testing against the next minor. This is trivial to do on GitLab CI ever since #3397129: Allow modules to opt in to testing against Drupal previous major, previous minor, next minor.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork webform-3402134

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Wim Leers created an issue. See original summary.

Wim Leers opened merge request !380

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Status: Active » Needs review
Issue tags: +gitlabci-contrib

Done. Note that we could go much further — see https://git.drupalcode.org/project/big_pipe_sessionless/-/blob/2.x/.gitl... for example:

  # Opt in to testing current minor against max supported PHP version.
  OPT_IN_TEST_MAX_PHP: '1'
  # Opt in to testing previous & next minor (Drupal 10.0.x and 10.2.x).
  OPT_IN_TEST_PREVIOUS_MINOR: '1'
  OPT_IN_TEST_NEXT_MINOR: '1'
  # Opt in to testing $CORE_PREVIOUS_MAJOR (currently Drupal 9.5).
  OPT_IN_TEST_PREVIOUS_MAJOR: '1'
  # Opt in to testing $CORE_MAJOR_DEVELOPMENT (currently Drupal 11).
  OPT_IN_TEST_NEXT_MAJOR: '1'
jrockowitz’s picture

jrockowitz commented
Status: Needs review » Reviewed & tested by the community

This is awesome!!!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Do you want me to add #3 too? :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I didn't know about that flag, that is neat, BUT: The regular phpunit job on GitlabCI currently takes *30 minutes* for webform, because the contrib template currently uses zero concurrency/optimization unlike core and this module has a *lot* of tests.

That makes running *every* pipeline against one or even multiple extra core versions very expensive.

It is possible to manually run a pipeline against 10.2.x, that's what I did for #3400868: [PP-2] to_email and similar config keys have schema type email but allow other things where I linked those results as well.

Not a maintainer, but IMHO it would be better to do it like core does, where alternative phpunit jobs are available on every MR to be started manually, because doing it through the UI as with custom variables is tedious.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » wim leers
Status: Reviewed & tested by the community » Needs work
Related issues: +#3370952: Run phpunit tests from a single job in parallel, using paratest

The regular phpunit job on GitlabCI currently takes *30 minutes* for webform

🤯

Yeah we should improve that so that it is improved for all contrib modules. But isn't it as simple as setting _PHPUNIT_CONCURRENT: "1" per #3370952: Run phpunit tests from a single job in parallel, using paratest? Let's find out.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

IIRC I tried that and at least back when I tried that, that mode didn't suppress deprecations.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

@Berdir fixed that using https://git.drupalcode.org/project/webform/-/merge_requests/380/diffs?co... 😊

Before #7
  • PHPUnit (current minor aka 10.1.x): 33.5 minutes
  • PHPUnit (next minor aka 10.2.x): 13 minutes (just so many early failures…)
After #7

(I spent far too long on that exact problem over at https://git.drupalcode.org/project/acquia_migrate/-/merge_requests/2, but I did find a solution at least! Glad to see it paying off! 😄)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Test Webform against upcoming 10.2.x on GitLab CI » Test Webform against upcoming 10.2.x on GitLab CI — and use concurrency to make tests 6x faster
Priority: Normal » Major
Issue tags: +DX (Developer Experience)

Thanks to @Berdir for nudging me in this direction 😇

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Off topic thoughts:

Ah, the extra trick is nice. and the performance improvement is impressive. Good to know. FWIW, this should be the default configuration for contrib IMHO, because I don't really want to adapt my 100 projects to have this overridden and risk breaking something in the future (sure, most contrib projects do not have remotely as many tests as webform). There's the meta issue about backporting core improvements to contrib, should be added there I think?. I assume it's OK to update the template and change how contrib tests work by default.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Test against 10.2.x HEAD
Results
❌ 
296) Drupal\Tests\webform\Kernel\Entity\WebformEntityTest::testWebformMethods
Exception: Exception when installing config for module webform, message was: Schema errors for webform.settings with the following errors: 0 [mail.default_to_mail] <em class="placeholder">"[site:mail]"</em> is not a valid email address., 1 [mail.default_from_mail] <em class="placeholder">"[site:mail]"</em> is not a valid email address.

/builds/issue/webform-3402134/web/core/tests/Drupal/KernelTests/KernelTestBase.php:732
/builds/issue/webform-3402134/tests/src/Kernel/Entity/WebformEntityTest.php:32
/builds/issue/webform-3402134/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

Caused by
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for webform.settings with the following errors: 0 [mail.default_to_mail] <em class="placeholder">"[site:mail]"</em> is not a valid email address., 1 [mail.default_from_mail] <em class="placeholder">"[site:mail]"</em> is not a valid email address.

/builds/issue/webform-3402134/web/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:94
/builds/issue/webform-3402134/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php:111
/builds/issue/webform-3402134/web/core/lib/Drupal/Core/Config/Config.php:229
/builds/issue/webform-3402134/web/core/lib/Drupal/Core/Config/ConfigInstaller.php:396
/builds/issue/webform-3402134/web/core/lib/Drupal/Core/Config/ConfigInstaller.php:149
/builds/issue/webform-3402134/web/core/lib/Drupal/Core/ProxyClass/Config/ConfigInstaller.php:75
/builds/issue/webform-3402134/web/core/tests/Drupal/KernelTests/KernelTestBase.php:729
/builds/issue/webform-3402134/tests/src/Kernel/Entity/WebformEntityTest.php:32
/builds/issue/webform-3402134/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
❌ 
222) Drupal\Tests\webform\Functional\States\WebformStatesManagerTest::testStatesToText
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for webform.settings with the following errors: 0 [mail.default_to_mail] <em class="placeholder">"[site:mail]"</em> is not a valid email address., 1 [mail.default_from_mail] <em class="placeholder">"[site:mail]"</em> is not a valid email address.

/builds/issue/webform-3402134/web/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:94
/builds/issue/webform-3402134/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php:111
/builds/issue/webform-3402134/web/core/lib/Drupal/Core/Config/Config.php:229
/builds/issue/webform-3402134/web/core/lib/Drupal/Core/Config/ConfigInstaller.php:396
/builds/issue/webform-3402134/web/core/lib/Drupal/Core/Config/ConfigInstaller.php:149
/builds/issue/webform-3402134/web/core/lib/Drupal/Core/ProxyClass/Config/ConfigInstaller.php:75
/builds/issue/webform-3402134/web/core/lib/Drupal/Core/Extension/ModuleInstaller.php:334
/builds/issue/webform-3402134/web/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
/builds/issue/webform-3402134/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:466
/builds/issue/webform-3402134/web/core/tests/Drupal/Tests/BrowserTestBase.php:559
/builds/issue/webform-3402134/web/core/tests/Drupal/Tests/BrowserTestBase.php:367
/builds/issue/webform-3402134/tests/src/Functional/WebformBrowserTestBase.php:44
/builds/issue/webform-3402134/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
Test against 10.2.x + #3402168: Follow-up for #3361534: Config validation errors can still occur for contrib modules, disrupting contrib
Results
✅ 
[32mDrupal\Tests\webform\Kernel\Entity\WebformEntityTest           3 passes
❌ 
[0mFATAL Drupal\Tests\webform\Functional\States\WebformStatesManagerTest: test runner returned a non-zero error code (2).
[31mDrupal\Tests\webform\Functional\States\WebformStatesManagerT   0 passes   1 fails

(Difference in output is due to switching from sequential to parallel test running per @Berdir in #6.

Conclusion

@alexpott was right in #3402168-12: Follow-up for #3361534: Config validation errors can still occur for contrib modules, disrupting contrib: the core MR does fix it for kernel tests, but not yet for functional tests.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2441811: Upgrade filter system to HTML5

Since #3402168-17: Follow-up for #3361534: Config validation errors can still occur for contrib modules, disrupting contrib, this is passing all of the 10.2.x tests 👍

With the exception of unrelated failures (for example, WebformStatesHiddenTest and WebformSettingsPreviewTest fail because it's asserting markup literally instead of using CSS selectors, XPath or DOMDocument — probably these output changes are due to #2441811: Upgrade filter system to HTML5). See the results.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Note: The alternative version this extends from was AFAIK merged and the patch committed, so this will need to be update to remove those things.

jrockowitz’s picture

jrockowitz commented

@Wim Leers & @Berdir Thank you for helping set this up.

The most immediate thing I did to help move this forward was to disable the old DrupaCI tests.

The fact that the 10.1.x tests are passing, and 6x faster is excellent. I am a little thrown off by the 10.2.x test failures but I am comfortable dealing with that in another ticket.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

> The most immediate thing I did to help move this forward was to disable the old DrupaCI tests.

Note that you then don't get testing of patches anymore.

DrupalCI isn't slow. GitlabCI without the changes here is slow. Now with those changes, GitlabCI is likely a bit faster than DrupalCI, not sure how much, but should not make a huge difference.

jrockowitz’s picture

jrockowitz commented

@Berdir Good catch. I restored the issue and commit test.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Test Webform against upcoming 10.2.x on GitLab CI — and use concurrency to make tests 6x faster » Test Webform against upcoming 10.2.x on GitLab CI — and use concurrency to make tests 6x faster (on GitLab CI)
Assigned: wim leers » Unassigned

To @Berdir's point: tweaking the title! :)

FYI: the DA will turn off DrupalCI at some point. But I agree that for a project like this, it may be better to just wait until GitLab CI contrib has been optimized further.

Also unassigning.

jrockowitz’s picture

jrockowitz commented
Status: Needs work » Closed (outdated)

_PHPUNIT_CONCURRENT: "1" has been added with additional improvement to drupalci.yml.