[9.x] Fix Carbon::setTestNow usage #40790
Conversation
| @@ -13,19 +13,11 @@ | |||
|
|
|||
| 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
| @@ -37,15 +44,13 @@ public function testMultipleItemsCanBeSetAndRetrieved() | |||
|
|
|||
| 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
| @@ -77,15 +82,13 @@ public function testNonExistingKeysCanBeIncremented() | |||
|
|
|||
| public function testExpiredKeysAreIncrementedLikeNonExistingKeys() | |||
| { | |||
| Carbon::setTestNow(Carbon::now()); | |||
There was a problem hiding this comment.
Same here, we dont want to compare a fake now
| @@ -11,20 +11,6 @@ | |||
|
|
|||
| 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.
| @@ -23,7 +23,8 @@ class CacheRepositoryTest extends TestCase | |||
| protected function tearDown(): void | |||
| { | |||
| 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
| @@ -26,6 +26,10 @@ class DatabaseEloquentBuilderTest extends TestCase | |||
| { | |||
| protected function tearDown(): void | |||
| { | |||
| parent::tearDown(); | |||
|
|
|||
| Carbon::setTestNow(null); | |||
There was a problem hiding this comment.
We can reset only once in the tearDown
| @@ -165,6 +167,8 @@ protected function tearDown(): void | |||
|
|
|||
| Relation::morphMap([], false); | |||
| Eloquent::unsetConnectionResolver(); | |||
|
|
|||
| Carbon::setTestNow(null); | |||
| @@ -2058,7 +2051,6 @@ public function testScopesMethod() | |||
| $model = new EloquentModelStub; | |||
| $this->addMockConnection($model); | |||
|
|
|||
| Carbon::setTestNow(); | |||
There was a problem hiding this comment.
Carbon is already reset to now anyway
| @@ -18,7 +18,7 @@ class DatabaseEloquentSoftDeletesIntegrationTest extends TestCase | |||
| { | |||
| protected function setUp(): void | |||
| { | |||
| Carbon::setTestNow(Carbon::now()); | |||
| @@ -22,7 +24,6 @@ protected function setUp(): void | |||
| $db->setAsGlobal(); | |||
|
|
|||
| $this->createSchema(); | |||
| Carbon::setTestNow(Carbon::now()); | |||
| @@ -19,7 +19,6 @@ public function testDeletedAtIsAddedToCastsAsDefaultType() | |||
|
|
|||
| public function testDeletedAtIsCastToCarbonInstance() | |||
| { | |||
| Carbon::setTestNow(Carbon::now()); | |||
| @@ -37,8 +37,6 @@ public function testLocksCanBeAcquiredAndReleased() | |||
|
|
|||
| public function testLocksCanBlockForSeconds() | |||
| { | |||
| Carbon::setTestNow(); | |||
| @@ -27,7 +27,7 @@ public function testCanProperlyLogFailedJob() | |||
| return $uuid; | |||
| }); | |||
|
|
|||
| Carbon::setTestNow($now = CarbonImmutable::now()); | |||
| $now = CarbonImmutable::now(); | |||
There was a problem hiding this comment.
We only use now, without faking it
| @@ -36,7 +36,8 @@ class ValidationValidatorTest extends TestCase | |||
| { | |||
| protected function tearDown(): void | |||
| { | |||
| Carbon::setTestNow(); | |||
There was a problem hiding this comment.
Carbon::setTestNow is already called in one of the test
| @@ -4019,6 +4020,8 @@ public function testDateEqualsRespectsCarbonTestNowWhenParameterIsRelative() | |||
|
|
|||
| $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
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.