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

Failed test in Component/HttpFoundation/Tests/Session/Storage/Handler/MemcachedSessionHandlerTest.php #9797

Closed
remicollet opened this issue Dec 17, 2013 · 18 comments

Comments

Projects
None yet
5 participants
@remicollet
Copy link
Contributor

commented Dec 17, 2013

+ /usr/bin/phpunit --include-path ./src --exclude-group tty,benchmark -d date.timezone=UTC src/Symfony/Component/HttpFoundation
PHPUnit 3.7.28 by Sebastian Bergmann.

Configuration read from /dev/shm/BUILD/symfony-    b0b421908d569e5024372ded65857707c409e0f7/phpunit.xml

...............................................................  63 / 713 (  8%)
......................S........................................ 126 / 713 ( 17%)
......................S........................................ 189 / 713 ( 26%)
............................................................... 252 / 713 ( 35%)
............................................................... 315 / 713 ( 44%)
............................................................... 378 / 713 ( 53%)
............................................................... 441 / 713 ( 61%)
............................................................... 504 / 713 ( 70%)
............................................................... 567 / 713 ( 79%)
............................................E.................. 630 / 713 ( 88%)
...........................................S.S.S.....S.S..S..S. 693 / 713 ( 97%)
....................

Time: 7.06 seconds, Memory: 18.50Mb

There was 1 error:

1) Symfony\Component\HttpFoundation\Tests\Session\Storage\Handler\MemcachedSessionHandlerTest::testOpenSession
Declaration of Mock_Memcached_9659d902::get() should be compatible with Memcached::get($key, $cache_cb = NULL, &$cas_token = NULL, &$udf_flags = NULL)

/dev/shm/BUILD/symfony-b0b421908d569e5024372ded65857707c409e0f7/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MemcachedSessionHandlerTest.php:34

FAILURES!
Tests: 713, Assertions: 1554, Errors: 1, Skipped: 9.
@cordoval

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2013

your setup is wrong, i am inclined to think that you do not have the right versions of things. maybe memcache or such.

~ phpunit --include-path ./src --exclude-group tty,benchmark -d date.timezone=UTC src/Symfony/Component/HttpFoundation                                                    Luiss-MacBook-Pro-3 [5:47:38]
PHPUnit 3.7.28 by Sebastian Bergmann.

Configuration read from /Users/cordoval/Sites/dashboard/vendor/symfony/symfony/phpunit.xml.dist

...............................................................  63 / 734 (  8%)
............................................................... 126 / 734 ( 17%)
......................S........................................ 189 / 734 ( 25%)
............................................................... 252 / 734 ( 34%)
............................................................... 315 / 734 ( 42%)
............................................................... 378 / 734 ( 51%)
............................................................... 441 / 734 ( 60%)
............................................................... 504 / 734 ( 68%)
............................................................... 567 / 734 ( 77%)
........................................................SSSSSSS 630 / 734 ( 85%)
SSSS........................................................... 693 / 734 ( 94%)
.S.S.S.....S.S..S..S.....................

Time: 8.03 seconds, Memory: 15.50Mb

OK, but incomplete or skipped tests!
Tests: 734, Assertions: 1580, Skipped: 19.

but also sounds like you have less skipped tests than i have. Which version of symfony are you running exactly? which versions or enabled things you have.

 phpunit --include-path ./src --exclude-group tty,benchmark -d date.timezone=UTC src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MemcachedSessionHandlerTest.php
PHPUnit 3.7.28 by Sebastian Bergmann.

Configuration read from /Users/cordoval/Sites/dashboard/vendor/symfony/symfony/phpunit.xml.dist

SSSSSSSSSSS

Time: 19 ms, Memory: 2.75Mb

OK, but incomplete or skipped tests!
Tests: 11, Assertions: 0, Skipped: 11.

Checking now enabling the thing. After brew install php55-memcached:

~ phpunit --include-path ./src --exclude-group tty,benchmark -d date.timezone=UTC src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MemcachedSessionHandlerTest.php
PHPUnit 3.7.28 by Sebastian Bergmann.

Configuration read from /Users/cordoval/Sites/dashboard/vendor/symfony/symfony/phpunit.xml.dist

...........

Time: 34 ms, Memory: 3.25Mb

OK (11 tests, 14 assertions)

I think is safe to close if your environment after updating works.

@remicollet

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2013

Yes this is related to new memcached extension version.
$ pecl list | grep memcached
memcached 2.1.0 stable

 $ phpunit --include-path ./src --exclude-group tty,benchmark -d date.timezone=UTC src/Symfony/Component     /HttpFoundation/Tests/Session/Storage/Handler/MemcachedSessionHandlerTest.php
 PHPUnit 3.7.28 by Sebastian Bergmann.

 Configuration read from /home/rpmbuild/SPECS/remirepo/php/symfony2/php-symfony/symfony-b0b421908d569e5024372ded65857707c409e0f7/phpunit.xml.dist

 ..........

 Time: 39 ms, Memory: 4.00Mb

This latest beta version:

 $ pecl list | grep memcached
 memcached      2.2.0b1  beta

 $ phpunit --include-path ./src --exclude-group tty,benchmark -d date.timezone=UTC src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MemcachedSessionHandlerTest.php
 PHPUnit 3.7.28 by Sebastian Bergmann.

 Configuration read from /home/rpmbuild/SPECS/remirepo/php/symfony2/php-symfony/symfony-b0b421908d569e5024372ded65857707c409e0f7/phpunit.xml.dist

 E.........

 Time: 25 ms, Memory: 4.00Mb

 There was 1 error:

 1) Symfony\Component\HttpFoundation\Tests\Session\Storage\Handler\MemcachedSessionHandlerTest::testOpenSession
 Declaration of Mock_Memcached_d41ef1ba::get() should be compatible with Memcached::get($key, $cache_cb = NULL, &$cas_token = NULL, &$udf_flags = NULL)
@remicollet

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2013

FYI,
php 5.5.7
symfony 2.3.8

Extracted from reflection diff:

 @@ -163,39 +227,43 @@

          Method [ <internal:memcached> public method get ] {

 -          - Parameters [3] {
 +          - Parameters [4] {
              Parameter #0 [ <required> $key ]
              Parameter #1 [ <optional> $cache_cb ]
              Parameter #2 [ <optional> &$cas_token ]
 +            Parameter #3 [ <optional> &$udf_flags ]
            }
          }
@cordoval

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2013

then please close as issue should be filed with them and not with symfony. I install the extension via brew install php55-memcached.

@remicollet

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2013

@cordoval sorry but I must disagree.

Yes memcached 2.2.0 is still beta and is not "yet" the default "stable" version.
This is obviously not a memcached bug, API have some new features, and I really don't think memcached upstream will revert them.

In a few time (weeks, months...) memcached new API will become the default "stable" one, and then symfony will need to be fixed.

I Test beta versions exactly for what they are designed => see what is broken and fix it.

@cordoval

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2013

I see so you plan to propose a PR for supporting this new API, provided you are totally sure this is the reason. I agree then. However it is a good idea to provide more details. Your first post did not even hint that you were using a beta version of the memcached extension. That said, it would be a good idea if you post all your research on a diff on the API.
Also notice that usually if they are using semver (from what i hear 👶 ) the API should not be break BC. right?

@remicollet

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2013

I see so you plan to propose a PR for supporting this new API,

No, I don't plan any PR, as I'm not enough aware of symfony internals.

However it is a good idea to provide more details.

Sorry, but I don't think at memcached beta when I open this report

That said, it would be a good idea if you post all your research on a diff on the API.

Also notice that usually if they are using semver (from what i hear 👶 ) the API should not be break BC. right?

I don't know if adding a new "optional" parameter is considered as a BC break. At least it doesn't break application using this API.

@fabpot

This comment has been minimized.

Copy link
Member

commented Dec 20, 2013

What I don't understand is why it fails as the new argument is optional.

@remicollet

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2013

It seems there is a bug in PHP with reflection and ARGINFO for internal extensions (which can be ZEND_SEND_BY_VAL , ZEND_SEND_BY_REF or ZEND_SEND_PREFER_REF).

And cas_token is ZEND_SEND_PREFER_REF.

I will try to report this to PHP project (out of my skill).

I think an acceptable workaround for the symfony test suite is to use, for this extension:

 @$this->memcached = $this->getMock('Memcached');

This works for me.

@cordoval

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2013

please provide the bug link @remicollet so i can document it

@remicollet

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2013

Just open: https://bugs.php.net/66331 ZEND_SEND_PREFER_REF issue trying to overlad

@cordoval

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2013

php#66331 great!

@remicollet

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2013

@cordoval @fabbot can you please consider the proposed workaround in previous comment, so this bug (which is not a symfony bug, but worth to be detected) can be closed).

cordoval added a commit to cordoval/symfony that referenced this issue Dec 20, 2013

@cordoval

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2013

@remicollet i am trying to setup the beta extension https://travis-ci.org/symfony/symfony/jobs/15771509 but it is holding on interactively i think

@krakjoe

This comment has been minimized.

Copy link

commented Dec 20, 2013

@cordoval

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2013

@krakjoe thanks for your participation in the community, we do appreciate your patch definitely!

@krakjoe

This comment has been minimized.

Copy link

commented Dec 21, 2013

It turns out there is some bigger problem, while the patch I submitted does fix the error and reflect the correct arg info in the engine, it would also allow function foo(&$bar = null) to be called as foo('bar'), which is something we purposely prohibit.

I'm not sure what the final solution will be here ... good to be moving forward anyway ...

fabpot added a commit that referenced this issue Dec 22, 2013

minor #9841 [Tests|WCM] add memcache, memcached, and mongodb extensio…
…ns to run skipped tests (cordoval)

This PR was merged into the 2.3 branch.

Discussion
----------

[Tests|WCM] add memcache, memcached, and mongodb extensions to run skipped tests

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

 - [x] go over all skipped tests, take note and check they are reasonable
 - [x] reenable memcache, mongodb, and memcached

We are keeping the icu intl related tests skipped because setting up icu 51.2 is totally time consuming in travis and it would require a custom distro box on travis because there are no ppa's available for the ubuntu version. I tried hard but it does not seem worth it. Same for plugging beta of memcached with pecl, it is just not reasonable to be running beta versions on travis. This then does not address #9797 but at least now we are aware.

This PR now can be merged as is as it improves tests that before were not ran. Not all but more than before. 👶

Commits
-------

47a822d add memcache, memcached, and mongodb extensions to run skipped tests

fabpot added a commit that referenced this issue Dec 22, 2013

minor #9830 [WCM][Tests] Improve tests running with specific requirem…
…ents (except intl icu) (cordoval)

This PR was submitted for the 2.4-dev branch but it was merged into the 2.4 branch instead (closes #9830).

Discussion
----------

[WCM][Tests] Improve tests running with specific requirements (except intl icu)

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

 - [x] go over all skipped tests, take note and check they are reasonable
 - [x] reenable memcache, mongodb, and memcached

We are keeping the icu intl related tests skipped because setting up icu 51.2 is totally time consuming in travis and it would require a custom distro box on travis because there are no ppa's available for the ubuntu version. I tried hard but it does not seem worth it. Same for plugging beta of memcached with pecl, it is just not reasonable to be running beta versions on travis. This then does not address #9797 but at least now we are aware.

This PR now can be merged as is as it improves tests that before were not ran. Not all but more than before. 👶

Commits
-------

6e0b2dc added condition to avoid skipping tests on JSON_PRETTY support
@mkoppanen

This comment has been minimized.

Copy link

commented Jan 16, 2014

Issue php-memcached-dev/php-memcached#126 tracks the upstream of this.

fabpot added a commit that referenced this issue Oct 23, 2014

minor #12297 enforce memcached version to be 2.1.0 (xabbuh)
This PR was merged into the 2.3 branch.

Discussion
----------

enforce memcached version to be 2.1.0

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

The signature of the `Memcached::get()` method changed with 2.2.0.
Therefore, tests fail in Symfony when mocking `Memcached` in the
`MemcachedSessionHandlerTest` of the HttpFoundation component (see
also php-memcached-dev/php-memcached#126 and
https://bugs.php.net/bug.php?id=66331).

Commits
-------

2ac5c86 enforce memcached version to be 2.1.0

@fabpot fabpot closed this Oct 23, 2014

fabpot added a commit that referenced this issue Sep 21, 2016

minor #20002 [HttpFoundation] Enable memcached tests with the latest …
…memcached extension (jakzal)

This PR was merged into the 2.7 branch.

Discussion
----------

[HttpFoundation] Enable memcached tests with the latest memcached extension

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

The check was introduced because of #9797. The issue is now fixed in the memcached extension, but only for the upcoming 3.0 release (for PHP7).

I've tested it with the latest memcached extension built from source and PHP 7.0.8.

Commits
-------

b482fb7 [HttpFoundation] Enable memcached tests with the latest memcached extension
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.