[9.x] Fix Carbon::setTestNow usage #40790
Conversation
| use stdClass; | ||
|
|
||
| class AuthDatabaseTokenRepositoryTest extends TestCase | ||
| { |
There was a problem hiding this comment.
We never need to pretent that Carbon::now is different than now in this test class
|
|
||
| public function testItemsCanExpire() | ||
| { | ||
| Carbon::setTestNow(Carbon::now()); |
There was a problem hiding this comment.
useless, Carbon will be now by default
| { | ||
| parent::tearDown(); | ||
|
|
||
| Carbon::setTestNow(null); |
There was a problem hiding this comment.
No need to reset multiple time after each test
|
|
||
| public function testExpiredKeysAreIncrementedLikeNonExistingKeys() | ||
| { | ||
| Carbon::setTestNow(Carbon::now()); |
There was a problem hiding this comment.
Same here, we dont want to compare a fake now
| class CacheFileStoreTest extends TestCase | ||
| { | ||
| protected function setUp(): void | ||
| { |
There was a problem hiding this comment.
We never need to pretent that Carbon::now is different than now in this test class
There was a problem hiding this comment.
@lucasmichot https://github.com/laravel/framework/runs/5798854285?check_suite_focus=true
This causes testIncrementDoesNotExtendCacheLife() to become brittle, intermittently failing if the second ticks over in the middle of this test case. Test setup Carbon::now()->addSeconds(50)->getTimestamp() and FileStore@put() -> InteractsWithTime@availableAt() code Carbon::now()->addRealSeconds($delay)->getTimestamp() must originate from the exact same Carbon::setTestNow() marker. When it doesn't, hash generation in the cache service can mismatch the setup test mock.
This brings into question every other Carbon::setTestNow(Carbon::now()) removal in this PR.
| m::close(); | ||
| Carbon::setTestNow(); | ||
|
|
||
| Carbon::setTestNow(null); |
There was a problem hiding this comment.
null is set for consistency sake.
| /* | ||
| * Use Carbon object... | ||
| */ | ||
| Carbon::setTestNow(Carbon::now()); |
There was a problem hiding this comment.
No fake now here
| { | ||
| parent::tearDown(); | ||
|
|
||
| Carbon::setTestNow(null); |
There was a problem hiding this comment.
We can reset only once in the tearDown
| Relation::morphMap([], false); | ||
| Eloquent::unsetConnectionResolver(); | ||
|
|
||
| Carbon::setTestNow(null); |
| $model = new EloquentModelStub; | ||
| $this->addMockConnection($model); | ||
|
|
||
| Carbon::setTestNow(); |
There was a problem hiding this comment.
Carbon is already reset to now anyway
| { | ||
| protected function setUp(): void | ||
| { | ||
| Carbon::setTestNow(Carbon::now()); |
| $db->setAsGlobal(); | ||
|
|
||
| $this->createSchema(); | ||
| Carbon::setTestNow(Carbon::now()); |
|
|
||
| public function testDeletedAtIsCastToCarbonInstance() | ||
| { | ||
| Carbon::setTestNow(Carbon::now()); |
|
|
||
| public function testLocksCanBlockForSeconds() | ||
| { | ||
| Carbon::setTestNow(); |
| }); | ||
|
|
||
| Carbon::setTestNow($now = CarbonImmutable::now()); | ||
| $now = CarbonImmutable::now(); |
There was a problem hiding this comment.
We only use now, without faking it
| { | ||
| protected function tearDown(): void | ||
| { | ||
| Carbon::setTestNow(); |
There was a problem hiding this comment.
Carbon::setTestNow is already called in one of the test
| $v = new Validator($trans, ['x' => new Carbon('2018-01-01')], ['x' => 'date_equals:tomorrow']); | ||
| $this->assertTrue($v->fails()); | ||
|
|
||
| Carbon::setTestNow(null); |
There was a problem hiding this comment.
No need to call Carbon::setTestNow in tearDown
GrahamCampbell
changed the title
3 participants
In many place in the tests, the usage of Carbon::setTestNow does not make sense or is not used properly.
This PR fixes that.
See PR code comments.
In a ideal world, a way to go would be to have an abstract test class that every other test (excepted integration) class could extend, this abstract class would close Mockery and reset Carbon in its tearDown method.