Analyze tests directory with psalm #2311
Conversation
| @@ -22,8 +22,9 @@ class UnitOfWorkPerformanceTest extends BaseTest | |||
| */ | |||
| public function testComputeChanges() | |||
| { | |||
| $n = 10000; | |||
There was a problem hiding this comment.
| @@ -17,11 +17,6 @@ public function __construct($name = null) | |||
| $this->name = $name; | |||
| } | |||
|
|
|||
| public function getId() | |||
| { | |||
| return $this->id; | |||
There was a problem hiding this comment.
id property is not in the class.
|
I got caught up making changes 😬 , I've tried to split it in commits and just change simple things that should be easy to review, otherwise let me know and I can split it in PRs. After this there are just 130 remaining issues in phpstan in tests. |
| @@ -518,7 +518,6 @@ public function testCommitsInProgressIsUpdatedOnException() | |||
| $this->expectExceptionMessage('This should not happen'); | |||
|
|
|||
| $this->dm->flush(); | |||
| $this->assertAttributeSame(0, 'commitsInProgress', $this->dm->getUnitOfWork()); | |||
There was a problem hiding this comment.
Can we replace this assertion in a different way, maybe by accessing the attribute in a bound closure?
There was a problem hiding this comment.
Hmmm the thing about this line is that it never gets executed because the flush() method throws an exception, but reading the test name: testCommitsInProgressIsUpdatedOnException maybe that exception should be catch and check the commitsInProgress property?
There was a problem hiding this comment.
On second thought, yes. The method should catch the exception, check the commitsInProgress property and then return. If no exception was caught, the test should fail using $this->fail().
There should be added to the mongodb stub of psalm, we are using findUnusedPsalmSuppress configuration option, so it will warn us when this is fixed.
franmomu
force-pushed
the
sa_tests
branch
2 times, most recently
from
0d2380c to
d1301c4
Compare
I've created #2317 |
| @@ -518,7 +518,6 @@ public function testCommitsInProgressIsUpdatedOnException() | |||
| $this->expectExceptionMessage('This should not happen'); | |||
|
|
|||
| $this->dm->flush(); | |||
| $this->assertAttributeSame(0, 'commitsInProgress', $this->dm->getUnitOfWork()); | |||
There was a problem hiding this comment.
On second thought, yes. The method should catch the exception, check the commitsInProgress property and then return. If no exception was caught, the test should fail using $this->fail().
|
Thanks @franmomu! |
Summary
Adding static analysis to
testsdirectory allow us to spot some issues.In this case with psalm since with phpstan there are quite a lot issues, I'll be creating PRs to try to fix them little by little (there were more than 300).