Skip to content

[Clock] Provide modify() in MockClock #48189

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

Merged
merged 1 commit into from
Nov 14, 2022

Conversation

dbrumann
Copy link
Contributor

@dbrumann dbrumann commented Nov 10, 2022

Q A
Branch? 6.2
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR -

This is mostly for DX, especially when handling longer sleep periods and when having to change the time multiple times on a single MockClock-instance, e.g. when using a service from a booted kernel in functional tests.

This PR provides a way to modify the MockClock's current time by specifying either a relative string like "+2 days", which will forward the current clock-time by 2 days, or an absolute datetime like "2112-09-17 23:53:03". It uses DateTimeImmutable::modify() and supports the same modifiers.

Example usage comparison between the already available sleep and modify:

$this->clock->sleep(172800); # 172800seconds = 48h

$this->clock->modify('48 hours');

@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "6.2" but it seems your PR description refers to branch "6.3".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@dbrumann dbrumann force-pushed the improvement/mock_clock branch from dd49d4b to 449c90d Compare November 10, 2022 14:23
@dbrumann
Copy link
Contributor Author

I figured it's a bit late to get this into 6.2, which is why I targeted 6.3 even though the branch is not there yet.

I know there were intense discussions about what should be part of the Clock in the original PR already, so feel free to close this if this functionality was omitted intentionally.

@dbrumann dbrumann force-pushed the improvement/mock_clock branch from 449c90d to 314d08b Compare November 10, 2022 15:24
@javiereguiluz
Copy link
Member

Thanks for this proposal @dbrumann.

In theory this addition looks reasonable ... but the Clock component has just been added. When things are so new, I think it's better to let people use it for some time. If something is annoying and requires a DX improvement, many people will complain about it and some will even report it here.

@wouterj
Copy link
Member

wouterj commented Nov 11, 2022

I think it makes sense to add this to the MockClock for 6.2. The DX looks much better 👍

@nicolas-grekas
Copy link
Member

I'm wondering if it wouldn't make more sense to expose a function that modifies the "now" while still using it as a reference.
It looks strange to me, and maybe not super useful, to pass a full datetime object. Eg your example in the description is not correct: it does not advance by 3 weeks. It sets the clock in three weeks from the current time.
In order to do what you describe, you should write this instead:

$this->clock->setTo($this->clock->now()->modify('3 weeks'));

If the first example is confusion already, maybe the API is not the best :)

What about a modify function instead?

function modify(string $modifier): void
{
	$this->now = $this->now->modifiy($modifier);
}

@OskarStark
Copy link
Contributor

What about a modify function instead?

I think both setNow() and modify() could be helpful.

Lets say you want to test, that on Christmas Eve an email is send and not before, its more handy to use the setNow(), as you know the specific time, while modify() could be used if you want to check something gets executed every X hours/days

WDYT?

@GromNaN
Copy link
Member

GromNaN commented Nov 11, 2022

The DateTimeImmutable::modify function accepts absolute values too.

var_dump((new DateTimeImmutable())->modify('2022-12-24'));
// 2022-12-24 10:49:34.003795

@OskarStark
Copy link
Contributor

The DateTimeImmutable::modify function accepts absolute values too.

TIL; thanks

@dbrumann
Copy link
Contributor Author

With modify we will have to handle invalid string inputs. I probably should introduce a custom Exception for this, right?

@nicolas-grekas
Copy link
Member

True. \InvalidArgumentException 💪

@dbrumann
Copy link
Contributor Author

dbrumann commented Nov 11, 2022

I used modify. I discovered that DateTimeImmutable::modify('Halloween'), i.e. with an invalid string, will throw an Error/Warning, which is why also added a try-block around it. Let me know, if you prefer having the internal message instead. It looks like this:

Testing /Users/dbr/Projects/symfony/symfony/src/Symfony/Component/Clock
.....E.........                                                   15 / 15 (100%)

Time: 00:01.527, Memory: 8.00 MB

There was 1 error:

1) Symfony\Component\Clock\Tests\MockClockTest::testModifyThrowsOnInvalidString
DateTimeImmutable::modify(): Failed to parse time string (Halloween) at position 0 (H): The timezone could not be found in the database

/Users/dbr/Projects/symfony/symfony/src/Symfony/Component/Clock/MockClock.php:52
/Users/dbr/Projects/symfony/symfony/src/Symfony/Component/Clock/Tests/MockClockTest.php:93

@GromNaN
Copy link
Member

GromNaN commented Nov 11, 2022

To fuel the discussion, I'm using something similar to the new clock component for some time and I never have to alter the current date after initialization.

  • For unit tests, the MockClock is initialized with the expected value.
  • For functional test (involving the kernel and clock as a service), I use an environment variable that is updated for the tests that need a specific value.

@OskarStark OskarStark changed the title [Clock] Provide setTo() in MockClock [Clock] Provide setTo() in MockClock Nov 11, 2022
@OskarStark OskarStark changed the title [Clock] Provide setTo() in MockClock [Clock] Provide modify() in MockClock Nov 11, 2022
@OskarStark
Copy link
Contributor

Can you please add a CHANGELOG entry?

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Just some minor details and good to me thanks.

@nicolas-grekas
Copy link
Member

Can you please also update the PR description btw?

@dbrumann dbrumann force-pushed the improvement/mock_clock branch from 8ebc7f8 to ea442bd Compare November 12, 2022 12:23
@dbrumann dbrumann force-pushed the improvement/mock_clock branch from ea442bd to 4d3ce8f Compare November 12, 2022 12:32
@nicolas-grekas
Copy link
Member

Thank you @dbrumann.

@nicolas-grekas nicolas-grekas merged commit 02193c0 into symfony:6.2 Nov 14, 2022
@dbrumann dbrumann deleted the improvement/mock_clock branch November 14, 2022 10:52
@fabpot fabpot mentioned this pull request Nov 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants