Problem

  • ExtensionDiscovery::getProfileDirectories() explicitly skips the simpletest parent installation profile processing in the installer, which seemingly defeats the entire purpose of that environment value:
         // For SimpleTest to be able to test modules packaged together with a
     // distribution we need to include the profile of the parent site (in
     // which test runs are triggered).
     // @todo !drupal_installation_attempted() defeats the whole purpose?
     if (drupal_valid_test_ua() && !drupal_installation_attempted()) {
       $testing_profile = \Drupal::config('simpletest.settings')->get('parent_profile');

    (Note the code comment.)

  • This means that additional modules that may be provided by an installation profile are not actually discovered when Drupal is installed in a web test.
CommentFileSizeAuthor
#32 2175391-31-test-only.patch1.28 KBrpayanm
#28 2175391-28.patch4.11 KBrpayanm
#25 2175391-25.patch4.13 KBrpayanm
#25 2175391-interdiff.txt1.1 KBrpayanm
#23 2175391-23.patch4.13 KBrpayanm
#20 test.parent-profile.17.test-only.patch1.33 KBsun
#17 test.parent-profile.17.patch4.45 KBsun
#16 drupal8.test-parent-profile.16.patch3.11 KBsun
#13 drupal8.ext-disco-parent-profile-2175391-4.patch3.15 KBBerdir
#6 drupal8.test-parent-profile.6.patch3.18 KBsun
#4 drupal8.ext-disco-parent-profile.4.patch880 bytessun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

So I looked and we have InstallationProfileModuleTestsTest which sort of tests this behavior. I'm not 100% sure, though. Can you elaborate why that test is not sufficient?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

I wasn't aware that there is a InstallationProfileModuleTestsTest for this.

Need to study the situation some more.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Hah. I actually authored that test myself ;)

This facility was originally introduced in #911354: Tests in profiles/[name]/modules cannot be run and cannot use a different profile for running tests

The original patch did not contain the additional !drupal_installation_attempted() condition.

The important test is OtherInstallationProfileModuleTestsTest.

However, studying those tests, I've the impression that the assertions are very poor/fragile and can easily result in false-positive test results.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Component: base system » extension system
Status: Active » Needs review
FileSize
drupal8.ext-disco-parent-profile.4.patch880 bytes

For starters.

Status: Needs review » Needs work

The last submitted patch, 4: drupal8.ext-disco-parent-profile.4.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal8.test-parent-profile.6.patch3.18 KB

I see. So it looks like that condition was added when Config\InstallStorage started to throw exceptions when trying to read config files that do not exist.

As part of #2186491: [meta] D8 Extension System: Discovery/Listing/Info, I wanted to convert this into a setting either way, so let's simply do that here.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: SystemListingInfo::profiles() explicitly skips parent installation profile in installer, defeating its purpose? » ExtensionDiscovery::getProfileDirectories() explicitly skips parent installation profile in installer, defeating its purpose
Issue summary: View changes
Issue tags: +Testing system
Related issues: +#911354: Tests in profiles/[name]/modules cannot be run and cannot use a different profile for running tests, +#2186491: [meta] D8 Extension System: Discovery/Listing/Info
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community

Nice, verified that this fixes the bug that #911354: Tests in profiles/[name]/modules cannot be run and cannot use a different profile for running tests was supposed to fix but got re-introduced. I can run tests of a module in a profiles/some_profile/modules folder with this patch and it does not work on HEAD.

Changes look fine, much cleaner than when using config, no need to document this, as it's just used internally.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests
Related issues: +#2217781: Can only run tests from the installed profile

We need some tests here - also discovered #2217781: Can only run tests from the installed profile.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review

@alexpott: That is super-tricky...

In order to test this, we would need a second testing installation profile in core, which ships with a custom test module in its own ./modules subdirectory, and that module needs to ship with a test.

Only in that way, there can be a test class that is located in the module of testing2 profile, and which uses the testing profile for running tests, but still has access to the module code located in testing2 profile.

The already existing OtherInstallationProfileModuleTestsTest was supposed to cover this scenario, but given that we do not have a second testing profile in core, today I understand why that did not catch the problem (as outlined above).

I'm not sure whether we really want to have a second testing2 profile in core...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

6: drupal8.test-parent-profile.6.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 6: drupal8.test-parent-profile.6.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
drupal8.ext-disco-parent-profile-2175391-4.patch3.15 KB

Re-roll. Yeah, not sure how to do test this...

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

13: drupal8.ext-disco-parent-profile-2175391-4.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 13: drupal8.ext-disco-parent-profile-2175391-4.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal8.test-parent-profile.16.patch3.11 KB

Re-rolled.

Also studied the situation regarding testability some more:

We would not only have to introduce a second testing profile, this new testing profile would additionally have to specify a (testing) module from a different profile as a dependency (so that the module is actually required in the installer).

I.e., in essence:

core/profiles/testing2/testing2.info.yml:
---
dependencies:
  # Explicitly depend on a module located in the installation profile
  # of the parent site (test runner).
  - drupal_system_listing_compatible_test

A second testing profile just for testing this would be pretty heavy, IMO...

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
test.parent-profile.17.patch4.45 KB

Actually, I figured out a way to unit test this.

The last submitted patch, 16: drupal8.test-parent-profile.16.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Nice job on the test! Can you upload a test-only patch to prove it works?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
test.parent-profile.17.test-only.patch1.33 KB
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Status: Needs review » Needs work
Issue tags: +Needs re-roll

Needs reroll, and the test-only patch shouldn't pass without the fix.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Issue tags: -Needs re-roll +Needs reroll
rpayanm’s picture

rpayanm
he/him
Primary language Spanish
CreditAttribution: rpayanm commented
Status: Needs work » Needs review
FileSize
2175391-23.patch4.13 KB

Status: Needs review » Needs work

The last submitted patch, 23: 2175391-23.patch, failed testing.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
CreditAttribution: rpayanm commented
Status: Needs work » Needs review
FileSize
2175391-interdiff.txt1.1 KB
2175391-25.patch4.13 KB

trying again!

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
CreditAttribution: rpayanm commented
Issue tags: -Needs reroll
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Issue tags: -Needs tests 
+++ b/core/modules/simpletest/src/Tests/ParentProfileTest.php
@@ -0,0 +1,41 @@
\ No newline at end of file

Should be a new line at the end.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
CreditAttribution: rpayanm commented
FileSize
2175391-28.patch4.11 KB

fixed :)

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented

@rpayanm could you upload the test as a separate patch to illustrate the fix?

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
CreditAttribution: rpayanm commented

Testing is not my forte :(
I said it "fixed" so you told me in #27.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented

@rpayanm you can generate a patch of just the test using:

git diff 8.0.x core/modules/simpletest/src/Tests/ParentProfileTest.php

This is just to insure that the added test addresses the issue at hand.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
CreditAttribution: rpayanm commented
FileSize
2175391-31-test-only.patch1.28 KB

@jhedstrom thank you, here it is :)

Status: Needs review » Needs work

The last submitted patch, 32: 2175391-31-test-only.patch, failed testing.

Status: Needs work » Needs review

jhedstrom queued 32: 2175391-31-test-only.patch for re-testing.

mgifford queued 32: 2175391-31-test-only.patch for re-testing.

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented
Status: Needs review » Needs work

Patch no longer applies.

mpotter’s picture

mpotter CreditAttribution: mpotter at Phase2 commented
Version: 8.0.x-dev » 8.2.x-dev

Anybody know why this issue got stalled? It looks like the patch was tested but then this wasn't ever marked RTBC so it got lost in the cracks? I could re-roll the patch if necessary, but wonder how this special case for simpletest should be potentially combined with a more general handling of additional profile directories proposed in #1356276: Allow profiles to define a base/parent profile

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented

@mpotter I think it was stalled first when it needed re-rolling, but secondly, the 'test only' patch in #20 and #32 passed, indicating it isn't testing what we think it's testing (or if it is, the issue is somehow fixed).

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Needs work » Postponed (maintainer needs more info)
Issue tags: +Bug Smash Initiative

I skimmed the comments and see that in #38 this has either been fixed or the test is not testing what it should be. That comment was 7 years ago. There has been no further discussion here which may suggest that this has, indeed, been fixed.

I did some research and found that the code changed in the patch (not the test) was removed from core in #3081501: Remove TestSetupTrait::$originalProfile and test infrastructure which uses it. As for the test, there is similar testing done in \Drupal\Tests\system\Kernel\Common\SystemListingTest::testFileScanIgnoreDirectory which seems to be showing the modules in install profiles are discovered. So, this does appear to be outdated.

I will check in #bugsmash. For now, setting to PMNMI.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Postponed (maintainer needs more info) » Closed (outdated)

There has been no further discussion to suggest that this is still a problem. Therefor, closing as outdated based on #52.