New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update to PHPUnit 8 #7688
Update to PHPUnit 8 #7688
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch of the wrong namings and namespaces that you fixed in this PR 👍
tests/Doctrine/Tests/ORM/Repository/DefaultRepositoryFactoryTest.php
Outdated
Show resolved
Hide resolved
002cb84
to
2180788
Compare
@@ -412,7 +412,7 @@ public function testAcceptsEntityManagerInterfaceInstances() : void | |||
$classMetadataFactory->setEntityManager($entityManager); | |||
|
|||
// not really the cleanest way to check it, but we won't add a getter to the CMF just for the sake of testing. | |||
self::assertAttributeSame($entityManager, 'em', $classMetadataFactory); | |||
// self::assertAttributeSame($entityManager, 'em', $classMetadataFactory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -382,6 +378,6 @@ public function testResultCacheProfileCanBeRemovedViaSetter() : void | |||
$query->useResultCache(true); | |||
$query->setResultCacheProfile(); | |||
|
|||
self::assertAttributeSame(null, 'queryCacheProfile', $query); | |||
// self::assertAttributeSame(null, 'queryCacheProfile', $query); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll work on this during the weekend. |
fe9ed5d
to
d2fbd38
Compare
23ce741
to
d9a1781
Compare
d9a1781
to
5d14f03
Compare
5d14f03
to
d45beb7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me quite happy, thanks!
@@ -39,8 +38,8 @@ public function testIssue() : void | |||
$listener | |||
->expects($this->once()) | |||
->method(Events::postFlush) | |||
->will($this->returnCallback(static function () use ($uow, $test) { | |||
$test->assertAttributeEmpty('extraUpdates', $uow, 'ExtraUpdates are reset before postFlush'); | |||
->will($this->returnCallback(static function () use ($uow) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please simplify this to willReturnCallback()
?
@@ -171,4 +172,22 @@ protected function getSharedSecondLevelCacheDriverImpl() | |||
|
|||
return $this->secondLevelCacheDriverImpl; | |||
} | |||
|
|||
public static function assertAttributeEmpty(string $haystackAttributeName, $haystackClassOrObject, string $message = '') : void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd avoid porting these methods to our test case. We should rather find alternative ways to test - compare objects with assertEquals()
, for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was @jwage's idea: #7688 (comment).
I've replaced with assertEquals and methods calling as much as I can, but there are cases that we can't 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see... how many methods are using them?
Can we keep that somewhat contained? Maybe a trait that gets used only when it's really necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have nothing to add but a question about an assert. If the other change requests are done, this is 👍 from my side.
@@ -405,7 +401,7 @@ public function testResultCacheProfileCanBeRemovedViaSetter() : void | |||
$query->useResultCache(true); | |||
$query->setResultCacheProfile(); | |||
|
|||
self::assertAttributeSame(null, 'queryCacheProfile', $query); | |||
self::assertAttributeEmpty('queryCacheProfile', $query); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assert uses empty()
and changes behavior of a test compared to the previous used check of null
. But this property contains usually an object. Should we keep it that way?
Closing as I won't on this anymore. |
Using: