Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Adding the patch in #40 here: #2105583: Add some sane strictness to phpunit tests to catch risky tests we see risky tests including EntityManagerTest
You can reproduce this even without the above patch as follows.
php vendor/bin/phpunit -c core/phpunit.xml.dist --verbose --report-useless-tests --filter EntityManager
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.
Runtime: PHP 5.6.4-4ubuntu6.3 with Xdebug 2.2.6
Configuration: /.../core/phpunit.xml.dist
.............................................R......
Time: 4.46 seconds, Memory: 177.25Mb
There was 1 risky test:
1) Drupal\Tests\Core\Entity\EntityManagerTest::testOnFieldDefinitionUpdate
This test did not perform any assertions
OK, but incomplete, skipped, or risky tests!
Tests: 52, Assertions: 128, Risky: 1.
Digging in, it turns out that a lot of the tests aren't asserting everything they think they are because the phpunit method getMockClass() doesn't seem to work for settings up assertions. I can't find any documentation of this, or even any documentation of getMockClass() but it explains the why this test is "risky" despite asserting method calls.
You can see this in the fact that the mock assertion is even asserting the wrong number of arguments to onFieldDefinitionUpdate().
Proposed resolution
Remove usage of getMockClass and replace them with prophecied mocks.
Remaining tasks
N/A
User interface changes
N/A
API changes
N/A
Data model changes
N/A
RC review: Testing fixes.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | entitymanagertest-2597844-2.patch | 8.93 KB | neclimdul |
Comments
Comment #2
neclimdulPatch.
Comment #3
neclimdulComment #4
Mile23I looked at this one, too, but I don't know enough prophecy and can't learn it right now. I'd wager that it's best to use one mocking system or the other but not both. :-)
Also, this illustrates how bad an idea it is to hold mocks as class properties (eg
$this->entityManager;), because it's unclear where they came from, what has modified them, or what expectations they have. They should be either factory functions or defined in the test function.But that's just me griping and way out of scope here. :-)
I can't quite review the prophecy use, but I can verify that the patch works to make the strict error go away.
Comment #5
neclimdulIt was my initial suspicion it was from mixing. I think it turns out from what I can tell its a weakness in getMockClass() specifically. So it would have been happening before EntityManagerTest started using prophecy. I didn't dig into the details but because its just handing around a classname and the the mock is instantiated elsewhere, my suspicion is that the problem is that the mock never gets registered in the mockObjects list.
This test is the only place we use that method in our entire test suite and its easy enough to work around currently by mocking and pushing the storage handler into the test manager.
Will see if I can get a prophet to review the modified code. ;)
Comment #6
tim.plunkettLGTM. Getting rid of usages of getMockClass is always a good thing.
Thanks @neclimdul!
Comment #7
alexpottCommitted ff04d6a and pushed to 8.0.x. Thanks!