Skip to content
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.

Sign up for GitHub

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

[9.x] Fix Carbon::setTestNow usage #40790

Merged
merged 2 commits into from
Feb 3, 2022
Merged

[9.x] Fix Carbon::setTestNow usage #40790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7de8574
Fix Carbon::setTestNow usage.
lucasmichot Feb 3, 2022
b320c7f
Fix CS.
lucasmichot Feb 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions tests/Auth/AuthDatabaseTokenRepositoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,11 @@

class AuthDatabaseTokenRepositoryTest extends TestCase
{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We never need to pretent that Carbon::now is different than now in this test class

protected function setUp(): void
{
parent::setUp();

Carbon::setTestNow(Carbon::now());
}

protected function tearDown(): void
{
parent::tearDown();

m::close();
Carbon::setTestNow(null);
}

public function testCreateInsertsNewRecordIntoTable()
Expand Down
14 changes: 8 additions & 6 deletions tests/Cache/CacheArrayStoreTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@

class CacheArrayStoreTest extends TestCase
{
protected function tearDown(): void
{
parent::tearDown();

Carbon::setTestNow(null);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to reset multiple time after each test

}

public function testItemsCanBeSetAndRetrieved()
{
$store = new ArrayStore;
Expand Down Expand Up @@ -37,15 +44,13 @@ public function testMultipleItemsCanBeSetAndRetrieved()

public function testItemsCanExpire()
{
Carbon::setTestNow(Carbon::now());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useless, Carbon will be now by default

$store = new ArrayStore;

$store->put('foo', 'bar', 10);
Carbon::setTestNow(Carbon::now()->addSeconds(10)->addSecond());
$result = $store->get('foo');

$this->assertNull($result);
Carbon::setTestNow(null);
}

public function testStoreItemForeverProperlyStoresInArray()
Expand Down Expand Up @@ -77,15 +82,13 @@ public function testNonExistingKeysCanBeIncremented()

public function testExpiredKeysAreIncrementedLikeNonExistingKeys()
{
Carbon::setTestNow(Carbon::now());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, we dont want to compare a fake now

$store = new ArrayStore;

$store->put('foo', 999, 10);
Carbon::setTestNow(Carbon::now()->addSeconds(10)->addSecond());
$result = $store->increment('foo');

$this->assertEquals(1, $result);
Carbon::setTestNow(null);
}

public function testValuesCanBeDecremented()
Expand Down Expand Up @@ -134,7 +137,6 @@ public function testCannotAcquireLockTwice()

public function testCanAcquireLockAgainAfterExpiry()
{
Carbon::setTestNow(Carbon::now());
$store = new ArrayStore;
$lock = $store->lock('foo', 10);
$lock->acquire();
Expand All @@ -146,6 +148,7 @@ public function testCanAcquireLockAgainAfterExpiry()
public function testLockExpirationLowerBoundary()
{
Carbon::setTestNow(Carbon::now());

$store = new ArrayStore;
$lock = $store->lock('foo', 10);
$lock->acquire();
Expand All @@ -156,7 +159,6 @@ public function testLockExpirationLowerBoundary()

public function testLockWithNoExpirationNeverExpires()
{
Carbon::setTestNow(Carbon::now());
$store = new ArrayStore;
$lock = $store->lock('foo');
$lock->acquire();
Expand Down
14 changes: 0 additions & 14 deletions tests/Cache/CacheFileStoreTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,6 @@

class CacheFileStoreTest extends TestCase
{
protected function setUp(): void
{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We never need to pretent that Carbon::now is different than now in this test class

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

parent::setUp();

Carbon::setTestNow(Carbon::now());
}

protected function tearDown(): void
{
parent::tearDown();

Carbon::setTestNow(null);
}

public function testNullIsReturnedIfFileDoesntExist()
{
$files = $this->mockFilesystem();
Expand Down
2 changes: 1 addition & 1 deletion tests/Cache/CacheMemcachedStoreTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public function testSetMethodProperlyCallsMemcache()
$store = new MemcachedStore($memcache);
$result = $store->put('foo', 'bar', 60);
$this->assertTrue($result);
Carbon::setTestNow();
Carbon::setTestNow(null);
}

public function testIncrementMethodProperlyCallsMemcache()
Expand Down
8 changes: 2 additions & 6 deletions tests/Cache/CacheRepositoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ class CacheRepositoryTest extends TestCase
protected function tearDown(): void
{
m::close();
Carbon::setTestNow();

Carbon::setTestNow(null);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null is set for consistency sake.

}

public function testGetReturnsValueFromCache()
Expand Down Expand Up @@ -96,11 +97,6 @@ public function testRememberMethodCallsPutAndReturnsDefault()
});
$this->assertSame('bar', $result);

/*
* Use Carbon object...
*/
Carbon::setTestNow(Carbon::now());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No fake now here


$repo = $this->getRepository();
$repo->getStore()->shouldReceive('get')->times(2)->andReturn(null);
$repo->getStore()->shouldReceive('put')->once()->with('foo', 'bar', 602);
Expand Down
4 changes: 2 additions & 2 deletions tests/Console/Scheduling/FrequencyTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ class FrequencyTest extends TestCase

protected function setUp(): void
{
Carbon::setTestNow();

$this->event = new Event(
m::mock(EventMutex::class),
'php foo'
Expand Down Expand Up @@ -102,6 +100,8 @@ public function testLastDayOfMonth()
Carbon::setTestNow('2020-10-10 10:10:10');

$this->assertSame('0 0 31 * *', $this->event->lastDayOfMonth()->getExpression());

Carbon::setTestNow(null);
}

public function testTwiceMonthly()
Expand Down
10 changes: 4 additions & 6 deletions tests/Database/DatabaseEloquentBuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ class DatabaseEloquentBuilderTest extends TestCase
{
protected function tearDown(): void
{
parent::tearDown();

Carbon::setTestNow(null);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can reset only once in the tearDown


m::close();
}

Expand Down Expand Up @@ -1665,8 +1669,6 @@ public function testUpdate()

$result = $builder->update(['foo' => 'bar']);
$this->assertEquals(1, $result);

Carbon::setTestNow(null);
}

public function testUpdateWithTimestampValue()
Expand Down Expand Up @@ -1711,8 +1713,6 @@ public function testUpdateWithAlias()

$result = $builder->from('table as alias')->update(['foo' => 'bar']);
$this->assertEquals(1, $result);

Carbon::setTestNow(null);
}

public function testUpsert()
Expand All @@ -1736,8 +1736,6 @@ public function testUpsert()
$result = $builder->upsert([['email' => 'foo', 'name' => 'bar'], ['name' => 'bar2', 'email' => 'foo2']], ['email']);

$this->assertEquals(2, $result);

Carbon::setTestNow(null);
}

public function testWithCastsMethod()
Expand Down
28 changes: 4 additions & 24 deletions tests/Database/DatabaseEloquentIntegrationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ protected function createSchema()
*/
protected function tearDown(): void
{
parent::tearDown();

foreach (['default', 'second_connection'] as $connection) {
$this->schema($connection)->drop('users');
$this->schema($connection)->drop('friends');
Expand All @@ -165,6 +167,8 @@ protected function tearDown(): void

Relation::morphMap([], false);
Eloquent::unsetConnectionResolver();

Carbon::setTestNow(null);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

}

/**
Expand Down Expand Up @@ -1627,8 +1631,6 @@ public function testUpdatingChildModelTouchesParent()

$this->assertTrue($future->isSameDay($post->fresh()->updated_at), 'It is not touching model own timestamps.');
$this->assertTrue($future->isSameDay($user->fresh()->updated_at), 'It is not touching models related timestamps.');

Carbon::setTestNow($before);
}

public function testMultiLevelTouchingWorks()
Expand All @@ -1647,8 +1649,6 @@ public function testMultiLevelTouchingWorks()

$this->assertTrue($future->isSameDay($post->fresh()->updated_at), 'It is not touching models related timestamps.');
$this->assertTrue($future->isSameDay($user->fresh()->updated_at), 'It is not touching models related timestamps.');

Carbon::setTestNow($before);
}

public function testDeletingChildModelTouchesParentTimestamps()
Expand All @@ -1666,8 +1666,6 @@ public function testDeletingChildModelTouchesParentTimestamps()
$post->delete();

$this->assertTrue($future->isSameDay($user->fresh()->updated_at), 'It is not touching models related timestamps.');

Carbon::setTestNow($before);
}

public function testTouchingChildModelUpdatesParentsTimestamps()
Expand All @@ -1686,8 +1684,6 @@ public function testTouchingChildModelUpdatesParentsTimestamps()

$this->assertTrue($future->isSameDay($post->fresh()->updated_at), 'It is not touching model own timestamps.');
$this->assertTrue($future->isSameDay($user->fresh()->updated_at), 'It is not touching models related timestamps.');

Carbon::setTestNow($before);
}

public function testTouchingChildModelRespectsParentNoTouching()
Expand Down Expand Up @@ -1715,8 +1711,6 @@ public function testTouchingChildModelRespectsParentNoTouching()
$before->isSameDay($user->fresh()->updated_at),
'It is touching model own timestamps in withoutTouching scope, when it should not.'
);

Carbon::setTestNow($before);
}

public function testUpdatingChildPostRespectsNoTouchingDefinition()
Expand All @@ -1737,8 +1731,6 @@ public function testUpdatingChildPostRespectsNoTouchingDefinition()

$this->assertTrue($future->isSameDay($post->fresh()->updated_at), 'It is not touching model own timestamps when it should.');
$this->assertTrue($before->isSameDay($user->fresh()->updated_at), 'It is touching models relationships when it should be disabled.');

Carbon::setTestNow($before);
}

public function testUpdatingModelInTheDisabledScopeTouchesItsOwnTimestamps()
Expand All @@ -1759,8 +1751,6 @@ public function testUpdatingModelInTheDisabledScopeTouchesItsOwnTimestamps()

$this->assertTrue($future->isSameDay($post->fresh()->updated_at), 'It is touching models when it should be disabled.');
$this->assertTrue($before->isSameDay($user->fresh()->updated_at), 'It is touching models when it should be disabled.');

Carbon::setTestNow($before);
}

public function testDeletingChildModelRespectsTheNoTouchingRule()
Expand All @@ -1780,8 +1770,6 @@ public function testDeletingChildModelRespectsTheNoTouchingRule()
});

$this->assertTrue($before->isSameDay($user->fresh()->updated_at), 'It is touching models when it should be disabled.');

Carbon::setTestNow($before);
}

public function testRespectedMultiLevelTouchingChain()
Expand All @@ -1802,8 +1790,6 @@ public function testRespectedMultiLevelTouchingChain()

$this->assertTrue($future->isSameDay($post->fresh()->updated_at), 'It is touching models when it should be disabled.');
$this->assertTrue($before->isSameDay($user->fresh()->updated_at), 'It is touching models when it should be disabled.');

Carbon::setTestNow($before);
}

public function testTouchesGreatParentEvenWhenParentIsInNoTouchScope()
Expand All @@ -1824,8 +1810,6 @@ public function testTouchesGreatParentEvenWhenParentIsInNoTouchScope()

$this->assertTrue($before->isSameDay($post->fresh()->updated_at), 'It is touching models when it should be disabled.');
$this->assertTrue($future->isSameDay($user->fresh()->updated_at), 'It is touching models when it should be disabled.');

Carbon::setTestNow($before);
}

public function testCanNestCallsOfNoTouching()
Expand All @@ -1848,8 +1832,6 @@ public function testCanNestCallsOfNoTouching()

$this->assertTrue($before->isSameDay($post->fresh()->updated_at), 'It is touching models when it should be disabled.');
$this->assertTrue($before->isSameDay($user->fresh()->updated_at), 'It is touching models when it should be disabled.');

Carbon::setTestNow($before);
}

public function testCanPassArrayOfModelsToIgnore()
Expand All @@ -1870,8 +1852,6 @@ public function testCanPassArrayOfModelsToIgnore()

$this->assertTrue($before->isSameDay($post->fresh()->updated_at), 'It is touching models when it should be disabled.');
$this->assertTrue($before->isSameDay($user->fresh()->updated_at), 'It is touching models when it should be disabled.');

Carbon::setTestNow($before);
}

public function testWhenBaseModelIsIgnoredAllChildModelsAreIgnored()
Expand Down
4 changes: 4 additions & 0 deletions tests/Database/DatabaseEloquentIrregularPluralTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,13 @@ public function createSchema()

protected function tearDown(): void
{
parent::tearDown();

$this->schema()->drop('irregular_plural_tokens');
$this->schema()->drop('irregular_plural_humans');
$this->schema()->drop('irregular_plural_human_irregular_plural_token');

Carbon::setTestNow(null);
}

protected function schema()
Expand Down
9 changes: 0 additions & 9 deletions tests/Database/DatabaseEloquentModelTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,6 @@ class DatabaseEloquentModelTest extends TestCase
{
use InteractsWithTime;

protected function setUp(): void
{
parent::setUp();

Carbon::setTestNow(Carbon::now());
}

protected function tearDown(): void
{
parent::tearDown();
Expand Down Expand Up @@ -2058,8 +2051,6 @@ public function testScopesMethod()
$model = new EloquentModelStub;
$this->addMockConnection($model);

Carbon::setTestNow();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carbon is already reset to now anyway


$scopes = [
'published',
'category' => 'Laravel',
Expand Down
2 changes: 1 addition & 1 deletion tests/Database/DatabaseEloquentSoftDeletesIntegrationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class DatabaseEloquentSoftDeletesIntegrationTest extends TestCase
{
protected function setUp(): void
{
Carbon::setTestNow(Carbon::now());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useless

parent::setUp();

$db = new DB;

Expand Down
3 changes: 2 additions & 1 deletion tests/Database/DatabaseEloquentTimestampsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ class DatabaseEloquentTimestampsTest extends TestCase
{
protected function setUp(): void
{
parent::setUp();

$db = new DB;

$db->addConnection([
Expand All @@ -22,7 +24,6 @@ protected function setUp(): void
$db->setAsGlobal();

$this->createSchema();
Carbon::setTestNow(Carbon::now());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useless

}

/**
Expand Down
1 change: 0 additions & 1 deletion tests/Database/DatabaseSoftDeletingTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ public function testDeletedAtIsAddedToCastsAsDefaultType()

public function testDeletedAtIsCastToCarbonInstance()
{
Carbon::setTestNow(Carbon::now());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useless

$expected = Carbon::createFromFormat('Y-m-d H:i:s', '2018-12-29 13:59:39');
$model = new SoftDeletingModel(['deleted_at' => $expected->format('Y-m-d H:i:s')]);

Expand Down
Loading