Problem/Motivation

UnitTestCase has a dependency on the block module, to provide a block entity for mocking.

It needs this for the getBlockMockWithMachineName() method, which is only used by one test in core: Drupal\Tests\block\Unit\BlockFormTest.

UnitTestCase should not have dependencies on extensions.

This currently works because both our bootstrap.php and run-tests.sh sets all module src/ directories to be autoloaded. This might change.

Proposed resolution

Deprecate UTC::getBlockMockWithMachineName(). Don't provide an alternative, because tests should be generating their own mocks at the test-class level, not at the base level.

Move the behavior of getBlockMockWithMachineName() to BlockFormTest, the only unit test class which uses it.

Remaining tasks

Follow-up to convert test mocking to use the ::class pattern.

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#22 interdiff.txt2.37 KBmile23
#22 2825078_22.patch3.69 KBmile23
#16 interdiff.txt686 bytesmile23
#16 2825078_16.patch3.67 KBmile23
#12 interdiff.txt7.74 KBmile23
#12 2825078_12.patch3.62 KBmile23
#10 2825078_10.patch7.2 KBmile23
#2 2825078_2.patch7.21 KBmile23

Comments

Mile23 created an issue. See original summary.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue summary: View changes
StatusFileSize
new 2825078_2.patch7.21 KB
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Active » Needs review
almaudoh’s picture

almaudoh commented

+1. Nice catch @Mile23

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Figure out how to limit autoloading to enabled modules for the purposes of kernel tests.

The simplest answer here is don't futz with the autoloader during testing :)

dawehner’s picture

dawehner
Primary language German
commented

Are we okay with this potential BC break? I guess yes, because its a clear bug.

almaudoh’s picture

almaudoh commented

Move getBlockMockWithMachineName() to BlockFormTest, the only class which uses it.

In general, this may also be applicable to helper methods that have been added to UnitTestCase just to support a handful other tests, e.g.
- getContainerWithCacheTagsInvalidator(): used by ViewsDataTest, CacheCollectorTest, ThemeHandlerTest and DefaultPluginManagerTest
- getClassResolverStub(): used by EntityResolverManagerTest, EntityTypeManagerTest and FormTestBase

These could better be moved to Traits. As a matter of fact, all the getXxxx() mocking helpers would be better moved to traits. A unit test should not be forced to have unneeded dependencies or unused classes autoloaded.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

@almaudoh: These could better be moved to Traits. As a matter of fact, all the getXxxx() mocking helpers would be better moved to traits. A unit test should not be forced to have unneeded dependencies or unused classes autoloaded.

For most uses, UnitTestCase only has dependencies on \Drupal and FileCacheFactory, both used in setUp(). Here's an issue about that: #2626832: Figure out how to check for unintentionally covered code

The unused classes aren't such a problem, and neither is having the getStubThingie() methods because if you don't use them they don't turn into hard dependencies of your test.

Turning the methods of UnitTestCase into a trait would just leave you with an empty class that uses a trait, and then no other use-case for the trait. If you need isolation from the dependencies in UnitTestCase you can just subclass \PHPUnit_Framework_TestCase, though of course there's some discussion about that: #2824313: All unit tests should use UnitTestCase base class

Re-running the test here.

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.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Title: UnitTestCase cleanup » UnitTestCase: Replace String Literals With ::class, Remove block Dependency
Parent issue: » #2807237: PHPUnit initiative
StatusFileSize
new 2825078_10.patch7.2 KB

Reroll of #2 for 8.4.x.

dawehner’s picture

dawehner
Primary language German
commented

For better reviewablity I would always suggest to do one thing at a time, in this case for example using ::class.
To minimize work for everyone involved there should be one issue then to use ::class in all unit tests, for example.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Title: UnitTestCase: Replace String Literals With ::class, Remove block Dependency » Remove UnitTestCase dependency on block module
Issue summary: View changes
StatusFileSize
new 2825078_12.patch3.62 KB
new interdiff.txt7.74 KB

To minimize work for everyone involved there should be one issue then to use ::class in all unit tests, for example.

That would not minimize work for whoever writes the patch. :-)

Adjusting scope based on #11.

The only problem we'll deal with here is that a) we should have deprecation and BC, while b) we also shouldn't have a hard requirement on an extension in a unit test base class.

Since that's the case, we @deprecate and @trigger_error() from UTC::getBlockMockWithMachineName().

We also move the behavior of getBlockMockWithMachineName() to BlockFormTest because base classes like UTC are not where special-case mocks should be set up.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Title: Remove UnitTestCase dependency on block module » Deprecate UnitTestCase dependency on block module
Issue tags: +@deprecated
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

That would not minimize work for whoever writes the patch. :-)

Well, as you know the lack of resources is more on the reviewer/committer side, than on the writing code side :)

I think the patch is ready as it is. I like the idea to not bloat up the UnitTestCase, if possible.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs work

Looks good except:

+++ b/core/tests/Drupal/Tests/UnitTestCase.php
@@ -180,6 +180,10 @@ public function getConfigStorageStub(array $configs) {
+   *
+   * @deprecated in Drupal 8.4.x, will be removed before Drupal 9.0.0. Unit test
+   *   base classes should not have dependencies on extensions. Set up mocks in
+   *   individual tests.

Needs to add a @see with a change notice link according to current best practices, so people can figure this out.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review
Related issues: +#2803621: Break BrowserTestBase & children dependency on Simpletest, deprecate stub BC traits
StatusFileSize
new 2825078_16.patch3.67 KB
new interdiff.txt686 bytes

This is similar to #2803621: Break BrowserTestBase & children dependency on Simpletest, deprecate stub BC traits which has a change record: https://www.drupal.org/node/2884454

However, that change record deals with simpletest module and BTB.

And so I added one: https://www.drupal.org/node/2896072

And added a @see as per #15.

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.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Re-running the test for 8.5.x.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs review » Reviewed & tested by the community

@Mile23 thanks for the bump, lost track of this one :)

Feedback from #15 has been addressed, CR looks good, patch looks good.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Related issues: +#2866082: [Plan] Roadmap for Simpletest
wim leers’s picture

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

This has to go into 8.5 now because we're in the 8.4 RC phase.

+++ b/core/tests/Drupal/Tests/UnitTestCase.php
@@ -180,6 +180,12 @@ public function getConfigStorageStub(array $configs) {
+   * @deprecated in Drupal 8.4.x, will be removed before Drupal 9.0.0. Unit test

@@ -195,6 +201,7 @@ protected function getBlockMockWithMachineName($machine_name) {
+    @trigger_error(__METHOD__ . ' is deprecated in Drupal 8.4.x, will be removed before Drupal 9.0.0. Unit test base classes should not have dependencies on extensions. Set up mocks in individual tests.', E_USER_DEPRECATED);

+++ b/core/tests/Drupal/Tests/UnitTestCaseDeprecationTest.php
@@ -0,0 +1,22 @@
+   * @covers ::getBlockMockWithMachineName

… which means this comment+message need to be updated.

Once these are updated, back to RTBC!

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review
StatusFileSize
new 2825078_22.patch3.69 KB
new interdiff.txt2.37 KB

Done. Also edited the change record with the proper branch.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs review » Reviewed & tested by the community

Feedback has been addressed, back to RTBC

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Updating issue credits to include those who provided reviews that shaped the final patch

  • larowlan committed 5c503f4 on 8.5.x 
    Issue #2825078 by Mile23, dawehner, Wim Leers, Gábor Hojtsy: Deprecate...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

Committed as 5c503f4 and pushed to 8.5.x

Published change record

Status: Fixed » Closed (fixed)

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