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
Comment | File | Size | Author |
---|---|---|---|
#22 | interdiff.txt | 2.37 KB | Mile23 |
#22 | 2825078_22.patch | 3.69 KB | Mile23 |
#16 | interdiff.txt | 686 bytes | Mile23 |
#16 | 2825078_16.patch | 3.67 KB | Mile23 |
#12 | interdiff.txt | 7.74 KB | Mile23 |
Comments
Comment #2
Mile23Comment #3
Mile23Comment #4
almaudoh CreditAttribution: almaudoh commented+1. Nice catch @Mile23
Comment #5
alexpottThe simplest answer here is don't futz with the autoloader during testing :)
Comment #6
dawehnerAre we okay with this potential BC break? I guess yes, because its a clear bug.
Comment #7
almaudoh CreditAttribution: almaudoh commentedIn 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.Comment #8
Mile23For most uses,
UnitTestCase
only has dependencies on\Drupal
andFileCacheFactory
, both used insetUp()
. Here's an issue about that: #2626832: Figure out how to check for unintentionally covered codeThe 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 inUnitTestCase
you can just subclass\PHPUnit_Framework_TestCase
, though of course there's some discussion about that: #2824313: All unit tests should use UnitTestCase base classRe-running the test here.
Comment #10
Mile23Reroll of #2 for 8.4.x.
Comment #11
dawehnerFor 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.
Comment #12
Mile23That 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()
fromUTC::getBlockMockWithMachineName()
.We also move the behavior of
getBlockMockWithMachineName()
toBlockFormTest
because base classes like UTC are not where special-case mocks should be set up.Comment #13
Mile23Comment #14
dawehnerWell, 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.Comment #15
Gábor HojtsyLooks good except:
Needs to add a @see with a change notice link according to current best practices, so people can figure this out.
Comment #16
Mile23This 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.
Comment #18
Mile23Re-running the test for 8.5.x.
Comment #19
Lendude@Mile23 thanks for the bump, lost track of this one :)
Feedback from #15 has been addressed, CR looks good, patch looks good.
Comment #20
Mile23Comment #21
Wim LeersThis has to go into 8.5 now because we're in the 8.4 RC phase.
… which means this comment+message need to be updated.
Once these are updated, back to RTBC!
Comment #22
Mile23Done. Also edited the change record with the proper branch.
Comment #23
LendudeFeedback has been addressed, back to RTBC
Comment #24
larowlanUpdating issue credits to include those who provided reviews that shaped the final patch
Comment #26
larowlanCommitted as 5c503f4 and pushed to 8.5.x
Published change record