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

Comments

Mile23 created an issue. See original summary.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Title: Deprecate Drupal\simpletest\InstallerTestBase » Deprecate Drupal\simpletest\InstallerTestBase, convert children to BTB
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue summary: View changes
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue tags: +@deprecated
martin107’s picture

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 commented
Status: Active » Needs review
StatusFileSize
new 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 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 commented
Status: Needs work » Needs review
StatusFileSize
new 2978952-9.patch13.93 KB
new 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 commented
Status: Needs work » Needs review
StatusFileSize
new 2978952-11.patch41.71 KB
new 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
commented
Issue tags: +Needs reroll

Patch no longer applies.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
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), INDIA 🇮🇳
commented
Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 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 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
commented
Status: Needs work » Needs review
Issue tags: +phpunit initiative
StatusFileSize
new 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
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
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2978952-19-21.txt2.34 KB
new 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
commented
Status: Needs review » Reviewed & tested by the community

LGTM.

alexpott’s picture

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