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.

CommentFileSizeAuthor
#13 add_test_to_ensure-followup-2539682-13.patch637 bytesneclimdul
#9 add_test_to_ensure-2539682-9.patch1.15 KBborisson_
#9 interdiff.txt1.09 KBborisson_
#8 add_test_to_ensure-2539682-8.patch1.05 KBborisson_
#8 interdiff.txt2.46 KBborisson_
#5 add_test_to_ensure-2539682-5.patch1.4 KBborisson_
#5 interdiff.txt1.48 KBborisson_
#1 add_test_to_ensure-2539682-1.patch1.46 KBborisson_

Comments

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Active » Needs review
StatusFileSize
new add_test_to_ensure-2539682-1.patch1.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

wim leers
Location Ghent 🇧🇪🇪🇺
commented
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

aspilicious commented

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

dawehner’s picture

dawehner
Primary language German
commented 
  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

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
StatusFileSize
new interdiff.txt1.48 KB
new add_test_to_ensure-2539682-5.patch1.4 KB

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

aspilicious commented

So we don't need a webtest anymore?

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

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

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
StatusFileSize
new interdiff.txt2.46 KB
new add_test_to_ensure-2539682-8.patch1.05 KB

This is now a part of the already existing ComposerIntegrationTest.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
StatusFileSize
new interdiff.txt1.09 KB
new add_test_to_ensure-2539682-9.patch1.15 KB

I added some extra comments in the test.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Looks great.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
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

neclimdul
He/Him
commented
Status: Fixed » Needs review
StatusFileSize
new add_test_to_ensure-followup-2539682-13.patch637 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

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Fixed

Fixed as a duplicate here: #2597644: ComposerIntegrationTest::testAllModulesReplaced() doesn't test anything.

  • 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.