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

Mock date() in ClockMock #27890

Merged
merged 1 commit into from
Aug 24, 2018
Merged

Mock date() in ClockMock #27890

merged 1 commit into from
Aug 24, 2018

Conversation

dontub
Copy link

@dontub dontub commented Jul 7, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no (In case date() is mocked in some other way execution would fail because of redeclaration. Could be avoided with an extra function_exists() check. WDYT?)
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

As to the PHP documentation date() uses the value of time() as timestamp if none is given. date() has to be mocked in ClockMock as well for this still being true, otherwise \time() is used as default.

BTW: The msec part of microtime() has 8 fractional digits on my system, ClockMock returns only 6...

@dontub
Copy link
Author

dontub commented Jul 7, 2018

The test fails because ClockMock is used from the vendor directory. Shall I add require_once(__DIR__ . '/../ClockMock.php'); to ClockMockTest.php to avoid this?

@nicolas-grekas
Copy link
Member

Thanks for the PR @dontub
Since this is a new feature, it should be submitted on master.
That may help on the test suite (not sure, let's see)

@nicolas-grekas nicolas-grekas added this to the next milestone Jul 8, 2018
@dontub dontub changed the base branch from 2.8 to master July 9, 2018 09:42
@dontub
Copy link
Author

dontub commented Jul 9, 2018

I thought it is more bug fix than feature because it fixes unexpected behaviour, though I can understand if you see it as feature.

As to the PHP documentation date() uses the value of time() as timestamp
if none is given. date() has to be mocked in ClockMock as well for this
still being true, otherwise \time() is used as default.
@acasademont
Copy link
Contributor

acasademont commented Jul 12, 2018

Hi @nicolas-grekas and @dontub I detected a small bug in the microtime(false) function (it should have 8 decimal positions instead of 6). I don't know if I should send a specific PR just for that bug or add it to this one? Also...I have the problem of running the tests, it uses the ClockMock of the vendor directory, how did you end up fixing it?

https://3v4l.org/GYacF

Thanks!

@dontub
Copy link
Author

dontub commented Jul 13, 2018

@acasademont I also noticed this issue (see end of the initial comment). You can run the tests for the phpunit bridge directly from the Bridge/PhpUnit/ directory. Just run composer install there and then ./bin/simple-phpunit.

@acasademont
Copy link
Contributor

acasademont commented Aug 22, 2018

Oops sorry, missed the comment. @nicolas-grekas should we also fix that? I believe it might depend on the OS PHP is installed in? On Mac/LInux is 8 decimal positions

@nicolas-grekas
Copy link
Member

@acasademont fix welcome (it's always 8 decimals on all OSes so it's safe fixing unconditionally.)

@acasademont
Copy link
Contributor

@nicolas-grekas sure no problem! I was waiting for this one to be merged so that I could reuse the newly created tests. Although it's such a small fix maybe @dontub can include it in this PR...

@nicolas-grekas
Copy link
Member

Thank you @dontub.

@nicolas-grekas nicolas-grekas merged commit e8ba79a into symfony:master Aug 24, 2018
@nicolas-grekas
Copy link
Member

Now merged, fix welcome (on branch 2.8)

nicolas-grekas added a commit that referenced this pull request Aug 24, 2018
This PR was merged into the 4.2-dev branch.

Discussion
----------

Mock date() in ClockMock

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no (In case `date()` is mocked in some other way execution would fail because of redeclaration. Could be avoided with an extra `function_exists()` check. WDYT?)
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

As to the [PHP documentation](https://secure.php.net/manual/en/function.date.php) `date()` uses the value of `time()` as timestamp if none is given. `date()` has to be mocked in ClockMock as well for this still being true, otherwise `\time()` is used as default.

BTW: The msec part of `microtime()` has 8 fractional digits on my system, ClockMock returns only 6...

Commits
-------

e8ba79a [Bridge/PhpUnit] Mock date() in ClockMock
@ro0NL
Copy link
Contributor

ro0NL commented Aug 24, 2018

What about related funcs suchs as localtime(), strftime(), getdate(), strtotime()?

@nicolas-grekas
Copy link
Member

Nobody uses them :)

@javiereguiluz
Copy link
Member

I've created symfony/symfony-docs#10287 to document this new feature. Please, don't forget to create a doc issue for every new feature.

@dontub it'd be great if you could provide the docs for this feature. If you want to try it and need some help, we'll be glad to help you in the Symfony Docs repository. Thanks!

nicolas-grekas added a commit that referenced this pull request Oct 2, 2018
…ont)

This PR was merged into the 2.8 branch.

Discussion
----------

[PHPUnitBridge] Fix ClockMock microtime() format

| Q             | A
| ------------- | ---
| Branch?       | 2.8 <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | yes/no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | no    <!-- please add some, will be required by reviewers -->
| Fixed tickets |    <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | <!-- required for new features -->

This is a follow-up PR to #27890 to fix the `microtime` precision, it should be 8 decimals instead of the current 6 (see https://3v4l.org/GYacF)

The problem now is that due to the new tests the whole testsuite will fail if run from the main directory, as hhvm and 5.4 targets are doing, due to phpunit using the wrong `ClockMock` class. Tests for 7.1 and 7.2 pass because they `cd` into the component directory.

Commits
-------

e3732b6 [PHPUnitBridge] Fix microtime() format
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018
This was referenced Nov 3, 2018
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.

6 participants