Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Clock] Providemodify() 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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@dbrumann
Copy link
Contributor

@dbrumanndbrumann commentedNov 10, 2022
edited
Loading

QA
Branch?6.2
Bug fix?no
New feature?yes
Deprecations?no
Tickets-
LicenseMIT
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 usesDateTimeImmutable::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');

maxbeckers, wouterj, and chadyred reacted with thumbs up emoji
@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
Copy link
ContributorAuthor

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.

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

dbrumann reacted with thumbs up emoji

@wouterj
Copy link
Member

wouterj commentedNov 11, 2022
edited
Loading

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

GromNaN reacted with thumbs up emoji

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

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

@OskarStark
Copy link
Contributor

What about a modify function instead?

I think bothsetNow() andmodify() 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 thesetNow(), as you know the specific time, whilemodify() could be used if you want to check something gets executed every X hours/days

WDYT?

@GromNaN
Copy link
Member

GromNaN commentedNov 11, 2022
edited
Loading

TheDateTimeImmutable::modify function accepts absolute values too.

var_dump((newDateTimeImmutable())->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
ContributorAuthor

Withmodify 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
ContributorAuthor

dbrumann commentedNov 11, 2022
edited
Loading

I usedmodify. I discovered thatDateTimeImmutable::modify('Halloween'), i.e. with an invalid string, will throw an Error/Warning, which is why also added atry-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 MBThere was 1 error:1) Symfony\Component\Clock\Tests\MockClockTest::testModifyThrowsOnInvalidStringDateTimeImmutable::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 commentedNov 11, 2022
edited
Loading

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, theMockClock 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.

@OskarStarkOskarStark changed the title[Clock] Provide setTo() in MockClock[Clock] ProvidesetTo() in MockClockNov 11, 2022
@OskarStarkOskarStark changed the title[Clock] ProvidesetTo() in MockClock[Clock] Providemodify() in MockClockNov 11, 2022
@OskarStark
Copy link
Contributor

Can you please add a CHANGELOG entry?

Copy link
Member

@nicolas-grekasnicolas-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 reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

Thank you@dbrumann.

@nicolas-grekasnicolas-grekas merged commit02193c0 intosymfony:6.2Nov 14, 2022
@dbrumanndbrumann deleted the improvement/mock_clock branchNovember 14, 2022 10:52
@fabpotfabpot mentioned this pull requestNov 19, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@javiereguiluzjaviereguiluzjaviereguiluz left review comments

@wouterjwouterjwouterj left review comments

@OskarStarkOskarStarkOskarStark left review comments

@xabbuhxabbuhxabbuh left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

6.2

Development

Successfully merging this pull request may close these issues.

8 participants

@dbrumann@carsonbot@javiereguiluz@wouterj@nicolas-grekas@OskarStark@GromNaN@xabbuh

[8]ページ先頭

©2009-2025 Movatter.jp