Problem/Motivation

Thanks to @webflo at #2429617-233: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!), I was told to update core/composer.json's replace section. I had no idea that was necessary.

Proposed resolution

Prevent this by having a test.

Remaining tasks

Write test.

User interface changes

None.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

borisson_’s picture

Status: Active » Needs review
FileSize
1.46 KB

Added a webtest to see test this. This couldn't go in a unit test because system_rebuild_module_data needs a container.

Wim Leers’s picture

Issue tags: -Needs tests

Great, thanks! :)

I've only got nits.

  1. +++ b/core/modules/system/src/Tests/ComposerWebTest.php
    @@ -0,0 +1,49 @@
    +   * Test to see if all modules are in the replace array of core's composer.json
    

    Verify all modules are listed in the 'replace' section of core's composer.json.

  2. +++ b/core/modules/system/src/Tests/ComposerWebTest.php
    @@ -0,0 +1,49 @@
    +      if (strpos($module->getPathname(), '/tests/') === false && strpos($module->getPathname(), '/profiles/testing/') === false) {
    

    s/false/FALSE/

aspilicious’s picture

What happens when you installed a contrib module? Will the test fail?

dawehner’s picture

  1. +++ b/core/modules/system/src/Tests/ComposerWebTest.php
    @@ -0,0 +1,49 @@
    + * Contains \Drupal\action\Tests\ActionUninstallTest.
    ...
    +class ComposerWebTest extends WebTestBase {
    

    Let's fix the doc

  2. +++ b/core/modules/system/src/Tests/ComposerWebTest.php
    @@ -0,0 +1,49 @@
    +
    +    $modules = system_rebuild_module_data();
    +    $module_names = [];
    +
    +    foreach ($modules as $name => $module) {
    

    What about just scanning /core/modules ?

borisson_’s picture

Fixed the issues from #2 and #4.
Now we're just scanning /core/modules, this also addresses @aspilicious' concern about contrib modules.

aspilicious’s picture

So we don't need a webtest anymore?

borisson_’s picture

You're right, I can integrate this in the already existing unit test, let's do that.

borisson_’s picture

This is now a part of the already existing ComposerIntegrationTest.

borisson_’s picture

I added some extra comments in the test.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Looks great.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

More tests++

Committed 151da1a and pushed to 8.0.x. Thanks!

  • alexpott committed 151da1a on 8.0.x
    Issue #2539682 by borisson_, Wim Leers: Add test to ensure composer.json...
neclimdul’s picture

Status: Fixed » Needs review
FileSize
637 bytes

A good reason to re-enable #2105583: Add some sane strictness to phpunit tests to catch risky tests. This test doesn't assert anything. @see patch for why.

Mile23’s picture

Status: Needs review » Fixed

  • alexpott committed 151da1a on 8.1.x
    Issue #2539682 by borisson_, Wim Leers: Add test to ensure composer.json...

Status: Fixed » Closed (fixed)

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