Problem/Motivation

We have two InstallerTestBase classes: One extends WebTestBase, and the other extends BrowserTestBase.

We should deprecate the WebTestBase one.

It has two concrete subclasses which should be converted to use Drupal\FunctionalTests\Installer\InstallerTestBase:

  • Drupal\config\Tests\ConfigInstallProfileUnmetDependenciesTest
  • Drupal\config_translation\Tests\ConfigTranslationInstallTest

Drupal\system\Tests\Installer\ConfigAfterInstallerTestBase is an abstract subclass which is already deprecated.

Proposed resolution

Convert the tests.

Deprecate Drupal\simpletest\InstallerTestBase in favor of Drupal\FunctionalTests\Installer\InstallerTestBase.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#21 2978952-21.patch6.31 KBLendude
#21 interdiff-2978952-19-21.txt2.34 KBLendude
#19 2978952-19.patch6.05 KBLendude
#15 2978952-15.patch19.4 KByogeshmpawar
#11 interdiff-2978952-9-11.txt1.21 KBmartin107
#11 2978952-11.patch41.71 KBmartin107
#9 interdiff-2978952-6-9.txt7.64 KBmartin107
#9 2978952-9.patch13.93 KBmartin107
#6 2978952-6.patch1.81 KBmartin107
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Title: Deprecate Drupal\simpletest\InstallerTestBase » Deprecate Drupal\simpletest\InstallerTestBase, convert children to BTB
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Issue summary: View changes
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Issue tags: +@deprecated
martin107’s picture

martin107 CreditAttribution: martin107 commented
Assigned: Unassigned » martin107

After following along with the issue summary,

I think this has a sound basis, and is a good small parcel of work to clear our technical debt.
Thanks for marking this out...

I am going to work on this now.

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Status: Active » Needs review
FileSize
2978952-6.patch1.81 KB

I am seeing some local test errors, and specifically different between running

phpunit directly as compared to running core/scripts/run-tests.sh

So I am running testbot with a patch I think will fail just to see the authoritative bug report.

I have yet to mark things deprecated etc.

Status: Needs review » Needs work

The last submitted patch, 6: 2978952-6.patch, failed testing. View results

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented

I can see the problem ....

The fail comes from the mkdir in ConfigInstallProfileUnmetDependenciesTest::setup()

It is a case of a variable being used before being set!

$this->siteDirectory

setup will call parent::setup() as its last act .. where the variable is defined in a call to

$this->prepareDatabasePrefix() [ see TestSetupTrait ]

So things will have to be reordered as they are converted... no biggie... just more tomorrow.

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Status: Needs work » Needs review
FileSize
2978952-9.patch13.93 KB
interdiff-2978952-6-9.txt7.64 KB

The interdiff looks more complicated than it is... so I had better explain.

A) Added trigger and deprecation notices

B) I copied the setUp() function into ConfigInstallProfileUnmetDependenciesTest so that I can insert a single additional function call in the middle of the setup sequence.

    // Add a dependancy that cannot be met.
    $this->copyTestingOverrides();

which just contains the code that was previously in setup [ see my observaiton in #8 ]

Status: Needs review » Needs work

The last submitted patch, 9: 2978952-9.patch, failed testing. View results

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Status: Needs work » Needs review
FileSize
2978952-11.patch41.71 KB
interdiff-2978952-9-11.txt1.21 KB

I now cannot reproduce this error, but it makes sense to copyTestingOverrides after the chmod.

Status: Needs review » Needs work

The last submitted patch, 11: 2978952-11.patch, failed testing. View results

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Issue tags: +Needs reroll

Patch no longer applies.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location 🇮🇳 Pune(MH)
CreditAttribution: yogeshmpawar at QED42 commented
Assigned: martin107 » yogeshmpawar

Assigning myself for re-rolling patch because it's untouched from last 18 hours.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location 🇮🇳 Pune(MH)
CreditAttribution: yogeshmpawar at QED42 commented
Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2978952-15.patch19.4 KB

Re-rolled the patch against 8.6.x branch.

Status: Needs review » Needs work

The last submitted patch, 15: 2978952-15.patch, failed testing. View results

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented

reroll looks good... thanks yogeshmpawar

I have no idea about the test failure.

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.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at Dx Experts commented
Status: Needs work » Needs review
Issue tags: +phpunit initiative
FileSize
2978952-19.patch6.05 KB

Added a CR and a 'see' to the deprecation message.

Some unrelated stuff got into the patch in #11 et al., so took that out, and no interdiff because that really wouldn't help here, changes are to big and mostly unrelated.

Simplified ConfigInstallProfileUnmetDependenciesTest, it's a tricky one that! We can't actually test that UnmetDependenciesException gets thrown since it gets re-thrown as a standard \Exception by \Drupal\Core\Test\HttpClientMiddleware\TestHttpClientMiddleware, so testing for the message will have to suffice I think.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs review » Needs work

For more explicit exception checking we have Drupal\KernelTests\Core\Config\ConfigInstallTest::testDependencyChecking().

It doesn't check the case of profiles, though.

I think the message is specific enough to be useful here, especially since I'm bending my brain trying to figure out how to turn this into a kernel test without success. Maybe a smarter person would do better. :-)

  1. +++ b/core/modules/config/tests/src/Functional/ConfigInstallProfileUnmetDependenciesTest.php
@@ -84,12 +80,11 @@ protected function setUpSite() {
+      $this->assertContains('Configuration objects provided by <em class="placeholder">user</em> have unmet dependencies: <em class="placeholder">system.action.user_block_user_action (action)</em>', $this->expectedException->getMessage());

    How about adding an inline comment about why we're using Exception instead of UnmetDependenciesException. 

  2. +++ b/core/modules/simpletest/src/InstallerTestBase.php
@@ -2,6 +2,8 @@
+@trigger_error(__NAMESPACE__ . '\InstallerTestBase is deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0. Instead, use \Drupal\FunctionalTests\Installer\InstallerTestBase, see https://www.drupal.org/node/2988752.', E_USER_DEPRECATED);

@@ -13,6 +15,10 @@
+ * @deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0.

    8.7.0 now.

The rest looks good.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at Dx Experts commented
Status: Needs work » Needs review
FileSize
interdiff-2978952-19-21.txt2.34 KB
2978952-21.patch6.31 KB

@Mile23 thanks for the review. We can make the test for UnmetDependenciesException a little better since the class of the exception is added to the message, so we can test for that too.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs review » Reviewed & tested by the community

LGTM.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 8.7.x-dev » 8.6.x-dev
Status: Reviewed & tested by the community » Fixed

I moved it to 8.6.0 since having the tests aligned between 8.6.x and 8.7.x makes bug fixes easier.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

Committed and pushed 5ba032730b to 8.7.x and 741daa787a to 8.6.x. Thanks!

  • alexpott committed 5ba0327 on 8.7.x 
    Issue #2978952 by martin107, Lendude, yogeshmpawar, Mile23: Deprecate...

  • alexpott committed 741daa7 on 8.6.x 
    Issue #2978952 by martin107, Lendude, yogeshmpawar, Mile23: Deprecate...

Status: Fixed » Closed (fixed)

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