fix unused variable warning #16498
fix unused variable warning #16498
Conversation
PhpUnit tests coverage p2 PhpUnit tests coverage p3 PhpUnit tests coverage p4 PhpUnit tests coverage p5 PhpUnit tests coverage p6 PhpUnit tests coverage p7
| */ | ||
| public function testRegisterFalse($mode) | ||
| { | ||
| $this->assertNull(DeprecationErrorHandler::register($mode)); |
There was a problem hiding this comment.
this doesn't test anything. I don't think we should add tests to make test coverage higher, test coverage should not be the goal.
There was a problem hiding this comment.
It tests that this method works. For example - there was use of not defined variable in ClockMock::microtime. Such cases can be spotted with such tests.
There was a problem hiding this comment.
this creates global side effects (changing the error handler)
I'd remove this test entirely: DeprecationErrorHandler can't be unit tested but only functionally tested, which is already the case (because we use it quite extensively on the travis matrix, and it works)
| $this->assertEquals(0, ClockMock::sleep($sleep)); | ||
| $t2 = ClockMock::time(); | ||
|
|
||
| $this->assertLessThanOrEqual($sleep, $t2 - $t1); |
There was a problem hiding this comment.
assertSame should work and is more strict thus better, isn't it?
|
Thanks for review. For now I decided to just fix one bug. |
|
Thank you @eventhorizonpl. |
Hi,
This PR fixes a bug in ClockMock class.
Best regards,
Michal