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.

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] Revert some Carbon::setTestNow() removals #41810

Merged
merged 1 commit into from
Apr 3, 2022

Conversation

derekmd
Copy link
Contributor

@derekmd derekmd commented Apr 3, 2022

Revert about half of cleanup PR #40790 that removed "useless" calls to Carbon::setTestNow(). Now some tests are non-deterministic and can intermittently fail for new PRs being submitted:

e.g., https://github.com/laravel/framework/runs/5798854285

1) Illuminate\Tests\Cache\CacheFileStoreTest::testIncrementDoesNotExtendCacheLife
Expectation failed for method name is "put" when invoked 1 time(s)
Parameter 1 for invocation Illuminate\Filesystem\Filesystem::put('D:\a\framework\framework\test...da8a33', '1648892146i:2;', true) does not match expected value.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1648892145i:2;'
+'1648892146i:2;'

The mocked cache expiry was unexpectedly 1 second in the future.


  • Carbon::setTestNow(Carbon::now()) keeps time-based mocks and assertions deterministic. Otherwise if the clock's second ticks over mid-test, assertions and mockery checks can be run on an unexpected time of day.
  • tests/Validation/ValidationValidatorTest.php
    • Avoid putting Carbon::setTestNow(null) at the end of the test case method. If that test fails, Carbon::setTestNow(new Carbon('2018-01-01')) is now leaking into subsequent tests.
    • Worst case, wrap it in a try { /* assertions */ } finally { Carbon::setTestNow(null); }.

@derekmd derekmd force-pushed the revert-carbon-test-now-removals branch 3 times, most recently from 7bcd3ca to 04cbc15 Compare April 3, 2022 15:58
Without exact Carbon::now() timestamps, this causes some tests to
become non-deterministic and intermittently fail if the
clock's second ticks over mid-test.
@derekmd derekmd force-pushed the revert-carbon-test-now-removals branch from 04cbc15 to d8d5d24 Compare April 3, 2022 16:01
@taylorotwell taylorotwell merged commit 7d959d5 into laravel:9.x Apr 3, 2022
@derekmd derekmd deleted the revert-carbon-test-now-removals branch April 3, 2022 16:25
@driesvints
Copy link
Member

Ping @lucasmichot just FYI ^

@kylekatarnls
Copy link
Contributor

For the record, Carbon 3 will be a bit more strict on this.

Carbon::setTestNow(Carbon::now()); is about freezing the time to make as if the whole test method was running instantly.

Typical case would be things like this:

$repo->getStore()->shouldReceive('put')->once()->with('foo', 'bar', 602);
$result = $repo->remember('foo', Carbon::now()->addMinutes(10)->addSeconds(2), static fn () => 'bar');

(see #41824)

Here the moment to be chosen as the mocked "now" does not matter but it's important that the time is actually frozen during the test.

Between the moment you call Carbon::now() in the test and the moment this date will be read and converted into a number of seconds, few microseconds will elapse. Then difference between this passed DateTime and the new "now" reference will be something like 601.999123 seconds. With Carbon 2 microseconds was brutally truncated, so such a test with no time mock would succeed most of the time and fail if the microsecond unluckily overflow to previous second. In Carbon 3, it will be always take microseconds into account and truncate the result so systematic failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants