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

[PhpUnit] Mock clock on @group time-sensitive annotations #16194

Merged
merged 1 commit into from Oct 12, 2015

Conversation

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 10, 2015

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Instead of spreading the same clock mock code everywhere, let's create a test case that helps mocking the time related functions.

phpunit Outdated
@@ -43,6 +43,7 @@ if (!file_exists("$PHPUNIT_DIR/phpunit-$PHPUNIT_VERSION/phpunit")) {
passthru("$COMPOSER remove --no-update symfony/yaml");
passthru("$COMPOSER require --dev --no-update symfony/phpunit-bridge \">=2.8@dev\"");
passthru("$COMPOSER install --prefer-source --no-progress --ansi");
symlink(realpath('../../src/Symfony/Bridge/PhpUnit/ClockMockedTestCase.php'), './vendor/symfony/phpunit-bridge/ClockMockedTestCase.php');

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Oct 10, 2015

Author Member

This line should be removed when merging

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Oct 10, 2015

Thinking a bit more about this, I'm going to replace the test case by an annotation.
Status: needs work

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:clockmock branch from e66b70a to a997fbc Oct 11, 2015
nicolas-grekas added a commit that referenced this pull request Oct 11, 2015
…ekas)

This PR was merged into the 2.8 branch.

Discussion
----------

[PhpUnit] Auto-register SymfonyTestsListener

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

This allows removing the copy/pasted `<listeners>` tags in our phpunit.xml.dist files and opens for future enhancements (like #16194)

Commits
-------

9e2bb00 [PhpUnit] Auto-register SymfonyTestsListener
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:clockmock branch from a997fbc to ecb243e Oct 11, 2015
@nicolas-grekas nicolas-grekas changed the title [PhpUnitBridge] Add ClockMockedTestCase [PhpUnit] Mock time on @group clockMock annotations Oct 11, 2015
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:clockmock branch from ecb243e to 0ee16cb Oct 11, 2015
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Oct 11, 2015

Updated to use @group clockMock annotations
Status: needs review


/**
* @group clockMock

This comment has been minimized.

Copy link
@fabpot

fabpot Oct 11, 2015

Member

Not a big fan of the group name. Also, wouldn't it be better for it to be an annotation instead of a group?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Oct 11, 2015

Author Member

i'm open for suggestions :) using a group annotation makes the implementation a lot easier by reusing a bunch of existing caching logic. It also allows filtering these specific tests when running the test suite...

This comment has been minimized.

Copy link
@sstok

sstok Oct 11, 2015

Contributor

(clock, time)Related, requiresFixedTime, timeSensitive ?

The mocking of a clock is just a detail, it's more about what it is fixing.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Oct 11, 2015

Author Member

clockMockable?

This comment has been minimized.

Copy link
@fabpot

fabpot Oct 11, 2015

Member

I think I don't like having mock in there.

I do understand that using a group is much easier (I recently tried to add an annotation for BF :)), but that does not make it better (and not sure it makes sense to filter those tests).

Time sensitive sounds much better.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:clockmock branch 2 times, most recently from 72ed068 to 3e81e43 Oct 11, 2015
@nicolas-grekas nicolas-grekas changed the title [PhpUnit] Mock time on @group clockMock annotations [PhpUnit] Mock time on @group clockMockable annotations Oct 11, 2015
@stof

This comment has been minimized.

Copy link
Member

stof commented Oct 11, 2015

The phpunit-bridge dev requirement in all these packages needs to be upgraded as it relies on the new feature

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Oct 12, 2015

@stof in fact I removed all of them in 4032c88: now that our phpunit wrapper requires phpunit-bridge, they are not required anymore.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:clockmock branch from 3e81e43 to 34a0846 Oct 12, 2015
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Oct 12, 2015

Updated to use @group time-sensitive annotations. OK for you?

@nicolas-grekas nicolas-grekas changed the title [PhpUnit] Mock time on @group clockMockable annotations [PhpUnit] Mock clock on @group time-sensitive annotations Oct 12, 2015
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Oct 12, 2015

👍

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Oct 12, 2015

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 34a0846 into symfony:2.8 Oct 12, 2015
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
fabpot added a commit that referenced this pull request Oct 12, 2015
…ons (nicolas-grekas)

This PR was merged into the 2.8 branch.

Discussion
----------

[PhpUnit] Mock clock on @group time-sensitive annotations

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Instead of spreading the same clock mock code everywhere, let's create a test case that helps mocking the time related functions.

Commits
-------

34a0846 [PhpUnit] Mock clock on @group time-sensitive annotations
@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:clockmock branch Oct 12, 2015
@eventhorizonpl

This comment has been minimized.

@nicolas-grekas

Hi,

I'm working on a tests for ClockMock class

Undefined variable: now

/home/michal/projekty/symfony/src/Symfony/Bridge/PhpUnit/ClockMock.php:69
/home/michal/projekty/symfony/src/Symfony/Bridge/PhpUnit/Tests/ClockMockTest.php:67

    return sprintf("%0.6f %d\n", $now - (int) $now, (int) self::$now);

I'm not sure what should be returned here. Care to explain? I'll submit your bugfix with tests PR.

This comment has been minimized.

Copy link
Member Author

nicolas-grekas replied Nov 8, 2015

that's a bug, $now should be self::$now!

This comment has been minimized.

Copy link

eventhorizonpl replied Nov 8, 2015

return sprintf("%0.6f %d\n", self::$now - (int) self::$now, (int) self::$now); ?

This comment has been minimized.

Copy link
Member Author

nicolas-grekas replied Nov 8, 2015

yep

This comment has been minimized.

Copy link

eventhorizonpl replied Nov 8, 2015

ok, thanks. I'll fix it in my PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.