From 7c1d324f3038fc8b3f3428f504ba8f451dc44031 Mon Sep 17 00:00:00 2001 From: Daniele Alessandri Date: Fri, 24 Jul 2015 17:26:06 +0200 Subject: [PATCH] [tests] Apply some fixes and improvements and remove old stuff. --- phpunit.xml.dist | 7 +- phpunit.xml.travisci | 7 +- tests/PHPUnit/PredisProfileTestCase.php | 4 +- tests/PHPUnit/PredisTestCase.php | 89 +++++++------------ tests/Predis/ClientTest.php | 5 +- .../Distributor/EmptyRingExceptionTest.php | 2 +- .../Cluster/Distributor/HashRingTest.php | 2 +- .../Cluster/Distributor/KetamaRingTest.php | 2 +- tests/Predis/Command/CommandTest.php | 5 +- tests/Predis/Command/HyperLogLogAddTest.php | 3 +- tests/Predis/Command/HyperLogLogCountTest.php | 3 +- tests/Predis/Command/HyperLogLogMergeTest.php | 3 +- tests/Predis/Command/KeyMoveTest.php | 3 +- .../Command/KeyPreciseTimeToLiveTest.php | 10 +-- tests/Predis/Command/KeyTimeToLiveTest.php | 12 ++- .../Command/ListPopFirstBlockingTest.php | 2 - .../Command/ListPopLastBlockingTest.php | 2 - .../ListPopLastPushHeadBlockingTest.php | 2 - .../PubSubUnsubscribeByPatternTest.php | 8 +- tests/Predis/Command/ServerScriptTest.php | 1 - tests/Predis/Configuration/OptionsTest.php | 3 +- tests/Predis/Connection/FactoryTest.php | 1 + tests/Predis/Monitor/ConsumerTest.php | 7 +- .../Protocol/Text/ProtocolProcessorTest.php | 2 - 24 files changed, 80 insertions(+), 105 deletions(-) diff --git a/phpunit.xml.dist b/phpunit.xml.dist index dcac5301..d8f0b987 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -1,6 +1,11 @@ - + + tests/Predis/ diff --git a/phpunit.xml.travisci b/phpunit.xml.travisci index 6e485efa..3ddbb95f 100644 --- a/phpunit.xml.travisci +++ b/phpunit.xml.travisci @@ -1,6 +1,11 @@ - + + tests/Predis/ diff --git a/tests/PHPUnit/PredisProfileTestCase.php b/tests/PHPUnit/PredisProfileTestCase.php index 9bbd8be6..b26aad6d 100644 --- a/tests/PHPUnit/PredisProfileTestCase.php +++ b/tests/PHPUnit/PredisProfileTestCase.php @@ -240,11 +240,11 @@ abstract class PredisProfileTestCase extends PredisTestCase /** * @group disconnected - * @todo Could it be that objects passed to the return callback of a mocked - * method are cloned instead of being passed by reference? */ public function testSingleProcessor() { + // Could it be that objects passed to the return callback of a mocked + // method are cloned instead of being passed by reference? $argsRef = null; $processor = $this->getMock('Predis\Command\Processor\ProcessorInterface'); diff --git a/tests/PHPUnit/PredisTestCase.php b/tests/PHPUnit/PredisTestCase.php index ac871798..45318df2 100644 --- a/tests/PHPUnit/PredisTestCase.php +++ b/tests/PHPUnit/PredisTestCase.php @@ -19,9 +19,18 @@ use Predis\Profile; */ abstract class PredisTestCase extends \PHPUnit_Framework_TestCase { - protected $predisRequirements = array(); protected $redisServerVersion = null; + /** + * Sleep the test case with microseconds resolution. + * + * @param float $seconds Seconds to sleep. + */ + protected function sleep($seconds) + { + usleep($seconds * 1000000); + } + /** * Verifies that a Redis command is a valid Predis\Command\CommandInterface * instance with the specified ID and command arguments. @@ -202,59 +211,10 @@ abstract class PredisTestCase extends \PHPUnit_Framework_TestCase } /** - * @param string $expectedVersion Expected redis version. - * @param string $operator Comparison operator. - * @param callable $callback Callback for matching version. + * Returns the Redis server version required to run a @connected test from + * the @requiresRedisVersion annotation decorating a test method. * * @return string - * - * @throws \PHPUnit_Framework_SkippedTestError When expected redis version is not met - */ - protected function executeOnRedisVersion($expectedVersion, $operator, $callback) - { - $version = $this->getRedisServerVersion(); - $comparation = version_compare($version, $expectedVersion); - - if ($match = eval("return $comparation $operator 0;")) { - call_user_func($callback, $this, $version); - } - - return $match; - } - - /** - * @param string $expectedVersion Expected redis version. - * @param string $operator Comparison operator. - * @param callable $callback Callback for matching version. - * - * @return string - * - * @throws \PHPUnit_Framework_SkippedTestError When expected redis version is not met - */ - protected function executeOnProfileVersion($expectedVersion, $operator, $callback) - { - $profile = $this->getProfile(); - $comparation = version_compare($version = $profile->getVersion(), $expectedVersion); - - if ($match = eval("return $comparation $operator 0;")) { - call_user_func($callback, $this, $version); - } - - return $match; - } - - /** - * Sleep the test case with microseconds resolution. - * - * @param float $seconds Seconds to sleep. - */ - protected function sleep($seconds) - { - usleep($seconds * 1000000); - } - - /** - * */ protected function getRequiredRedisServerVersion() { @@ -271,7 +231,27 @@ abstract class PredisTestCase extends \PHPUnit_Framework_TestCase } /** + * Compares the specified version string against the Redis server version in + * use for integration tests. * + * @param string $operator Comparison operator. + * @param string $version Version to compare. + * + * @return bool + */ + public function isRedisServerVersion($operator, $version) + { + $serverVersion = $this->getRedisServerVersion(); + $comparation = version_compare($serverVersion, $version); + + return (bool) eval("return $comparation $operator 0;"); + } + + /** + * Checks that the Redis server version used to run integration tests mets + * the requirements specified with the @requiresRedisVersion annotation. + * + * @throws \PHPUnit_Framework_SkippedTestError When expected Redis server version is not met. */ protected function checkRequiredRedisServerVersion() { @@ -279,7 +259,6 @@ abstract class PredisTestCase extends \PHPUnit_Framework_TestCase return; } - $serverVersion = $this->getRedisServerVersion(); $requiredVersion = explode(' ', $requiredVersion, 2); if (count($requiredVersion) === 1) { @@ -290,9 +269,7 @@ abstract class PredisTestCase extends \PHPUnit_Framework_TestCase $reqVersion = $requiredVersion[1]; } - $comparation = version_compare($serverVersion, $reqVersion); - - if (!$match = eval("return $comparation $reqOperator 0;")) { + if (!$this->isRedisServerVersion($reqOperator, $reqVersion)) { $this->markTestSkipped( "This test requires Redis $reqOperator $reqVersion but the current version is $serverVersion." ); diff --git a/tests/Predis/ClientTest.php b/tests/Predis/ClientTest.php index e21d796e..02efe4ed 100644 --- a/tests/Predis/ClientTest.php +++ b/tests/Predis/ClientTest.php @@ -731,7 +731,6 @@ class ClientTest extends PredisTestCase /** * @group disconnected - * @todo I hate this test but reflection is the easiest way in this case. */ public function testTransactionWithArrayReturnsMultiExecTransactionWithOptions() { @@ -741,6 +740,7 @@ class ClientTest extends PredisTestCase $this->assertInstanceOf('Predis\Transaction\MultiExec', $tx = $client->transaction($options)); + // I hate this part but reflection is the easiest way in this case. $property = new ReflectionProperty($tx, 'modeCAS'); $property->setAccessible(true); $this->assertSame($options['cas'], $property->getValue($tx)); @@ -755,8 +755,7 @@ class ClientTest extends PredisTestCase */ public function testTransactionWithArrayAndCallableExecutesMultiExec() { - // NOTE: we use CAS since testing the actual MULTI/EXEC context - // here is not the point. + // We use CAS here as we don't care about the actual MULTI/EXEC context. $options = array('cas' => true, 'retry' => 3); $connection = $this->getMock('Predis\Connection\NodeConnectionInterface'); diff --git a/tests/Predis/Cluster/Distributor/EmptyRingExceptionTest.php b/tests/Predis/Cluster/Distributor/EmptyRingExceptionTest.php index 3e9d96f9..e6dbee55 100644 --- a/tests/Predis/Cluster/Distributor/EmptyRingExceptionTest.php +++ b/tests/Predis/Cluster/Distributor/EmptyRingExceptionTest.php @@ -14,7 +14,7 @@ namespace Predis\Cluster\Distributor; use PredisTestCase; /** - * @todo Not really useful right now. + * */ class EmptyRingExceptionTest extends PredisTestCase { diff --git a/tests/Predis/Cluster/Distributor/HashRingTest.php b/tests/Predis/Cluster/Distributor/HashRingTest.php index a64d2f19..33f9378f 100644 --- a/tests/Predis/Cluster/Distributor/HashRingTest.php +++ b/tests/Predis/Cluster/Distributor/HashRingTest.php @@ -12,7 +12,7 @@ namespace Predis\Cluster\Distributor; /** - * @todo To be improved. + * */ class HashRingTest extends PredisDistributorTestCase { diff --git a/tests/Predis/Cluster/Distributor/KetamaRingTest.php b/tests/Predis/Cluster/Distributor/KetamaRingTest.php index 0f641192..8d08f214 100644 --- a/tests/Predis/Cluster/Distributor/KetamaRingTest.php +++ b/tests/Predis/Cluster/Distributor/KetamaRingTest.php @@ -12,7 +12,7 @@ namespace Predis\Cluster\Distributor; /** - * @todo To be improved. + * */ class KetamaRingTest extends PredisDistributorTestCase { diff --git a/tests/Predis/Command/CommandTest.php b/tests/Predis/Command/CommandTest.php index 501b013e..49a574eb 100644 --- a/tests/Predis/Command/CommandTest.php +++ b/tests/Predis/Command/CommandTest.php @@ -54,9 +54,8 @@ class CommandTest extends PredisTestCase /** * @group disconnected * - * @todo We cannot set an expectation for Command::filterArguments when we - * invoke Command::setArguments() because it is protected. I wonder - * how we can do that. + * @todo We cannot set an expectation for Command::filterArguments() when we + * invoke Command::setArguments() because it is protected. */ public function testSetArguments() { diff --git a/tests/Predis/Command/HyperLogLogAddTest.php b/tests/Predis/Command/HyperLogLogAddTest.php index f45496c9..9076a126 100644 --- a/tests/Predis/Command/HyperLogLogAddTest.php +++ b/tests/Predis/Command/HyperLogLogAddTest.php @@ -14,7 +14,8 @@ namespace Predis\Command; /** * @group commands * @group realm-hyperloglog - * @todo Add integration tests depending on the minor redis version + * + * @todo Add integration tests depending on the minor redis version. */ class HyperLogLogAddTest extends PredisCommandTestCase { diff --git a/tests/Predis/Command/HyperLogLogCountTest.php b/tests/Predis/Command/HyperLogLogCountTest.php index c5434610..82185cae 100644 --- a/tests/Predis/Command/HyperLogLogCountTest.php +++ b/tests/Predis/Command/HyperLogLogCountTest.php @@ -14,7 +14,8 @@ namespace Predis\Command; /** * @group commands * @group realm-hyperloglog - * @todo Add integration tests depending on the minor redis version + * + * @todo Add integration tests depending on the minor redis version. */ class HyperLogLogCountTest extends PredisCommandTestCase { diff --git a/tests/Predis/Command/HyperLogLogMergeTest.php b/tests/Predis/Command/HyperLogLogMergeTest.php index 9bad2e70..4930d3bc 100644 --- a/tests/Predis/Command/HyperLogLogMergeTest.php +++ b/tests/Predis/Command/HyperLogLogMergeTest.php @@ -14,7 +14,8 @@ namespace Predis\Command; /** * @group commands * @group realm-hyperloglog - * @todo Add integration tests depending on the minor redis version + * + * @todo Add integration tests depending on the minor redis version. */ class HyperLogLogMergeTest extends PredisCommandTestCase { diff --git a/tests/Predis/Command/KeyMoveTest.php b/tests/Predis/Command/KeyMoveTest.php index dc9f31fb..c47a84ca 100644 --- a/tests/Predis/Command/KeyMoveTest.php +++ b/tests/Predis/Command/KeyMoveTest.php @@ -60,7 +60,8 @@ class KeyMoveTest extends PredisCommandTestCase /** * @group connected - * @todo This test fails if REDIS_SERVER_DBNUM is 0. + * + * @todo Should be improved, this test fails when REDIS_SERVER_DBNUM is 0. */ public function testMovesKeysToDifferentDatabases() { diff --git a/tests/Predis/Command/KeyPreciseTimeToLiveTest.php b/tests/Predis/Command/KeyPreciseTimeToLiveTest.php index 1a50f874..26f0fbba 100644 --- a/tests/Predis/Command/KeyPreciseTimeToLiveTest.php +++ b/tests/Predis/Command/KeyPreciseTimeToLiveTest.php @@ -83,13 +83,13 @@ class KeyPreciseTimeToLiveTest extends PredisCommandTestCase /** * @group connected - * @todo PTTL changed in Redis >= 2.8 to return -2 on non existing keys, we - * should handle this case with a better solution than the current one. */ public function testReturnsLessThanZeroOnNonExistingKeys() { - $redis = $this->getClient(); - - $this->assertLessThanOrEqual(-1, $redis->pttl('foo')); + if ($this->isRedisServerVersion('<', '2.8.0')) { + $this->assertSame(-1, $this->getClient()->pttl('foo')); + } else { + $this->assertSame(-2, $this->getClient()->pttl('foo')); + } } } diff --git a/tests/Predis/Command/KeyTimeToLiveTest.php b/tests/Predis/Command/KeyTimeToLiveTest.php index 1618cb3d..aceec3bb 100644 --- a/tests/Predis/Command/KeyTimeToLiveTest.php +++ b/tests/Predis/Command/KeyTimeToLiveTest.php @@ -86,12 +86,10 @@ class KeyTimeToLiveTest extends PredisCommandTestCase */ public function testReturnsLessThanZeroOnNonExistingKeys() { - $this->executeOnRedisVersion('2.8.0', '<', function ($test) { - $test->assertSame(-1, $test->getClient()->ttl('foo')); - }); - - $this->executeOnRedisVersion('2.8.0', '>=', function ($test) { - $test->assertSame(-2, $test->getClient()->ttl('foo')); - }); + if ($this->isRedisServerVersion('<', '2.8.0')) { + $this->assertSame(-1, $this->getClient()->ttl('foo')); + } else { + $this->assertSame(-2, $this->getClient()->ttl('foo')); + } } } diff --git a/tests/Predis/Command/ListPopFirstBlockingTest.php b/tests/Predis/Command/ListPopFirstBlockingTest.php index c1dc5f15..61c71f27 100644 --- a/tests/Predis/Command/ListPopFirstBlockingTest.php +++ b/tests/Predis/Command/ListPopFirstBlockingTest.php @@ -14,8 +14,6 @@ namespace Predis\Command; /** * @group commands * @group realm-list - * @todo Testing blocking pop operations against Redis using PHP is - * tricky, so we will skip these kind of tests for now. */ class ListPopFirstBlockingTest extends PredisCommandTestCase { diff --git a/tests/Predis/Command/ListPopLastBlockingTest.php b/tests/Predis/Command/ListPopLastBlockingTest.php index aed14aaf..4b79e62c 100644 --- a/tests/Predis/Command/ListPopLastBlockingTest.php +++ b/tests/Predis/Command/ListPopLastBlockingTest.php @@ -14,8 +14,6 @@ namespace Predis\Command; /** * @group commands * @group realm-list - * @todo Testing blocking pop operations against Redis using PHP is - * tricky, so we will skip these kind of tests for now. */ class ListPopLastBlockingTest extends PredisCommandTestCase { diff --git a/tests/Predis/Command/ListPopLastPushHeadBlockingTest.php b/tests/Predis/Command/ListPopLastPushHeadBlockingTest.php index fb0b71ab..d4c063e8 100644 --- a/tests/Predis/Command/ListPopLastPushHeadBlockingTest.php +++ b/tests/Predis/Command/ListPopLastPushHeadBlockingTest.php @@ -14,8 +14,6 @@ namespace Predis\Command; /** * @group commands * @group realm-list - * @todo Testing blocking pop operations against Redis using PHP is - * tricky, so we will skip these kind of tests for now. */ class ListPopLastPushHeadBlockingTest extends PredisCommandTestCase { diff --git a/tests/Predis/Command/PubSubUnsubscribeByPatternTest.php b/tests/Predis/Command/PubSubUnsubscribeByPatternTest.php index dfdea426..6e29a6ea 100644 --- a/tests/Predis/Command/PubSubUnsubscribeByPatternTest.php +++ b/tests/Predis/Command/PubSubUnsubscribeByPatternTest.php @@ -109,17 +109,13 @@ class PubSubUnsubscribeByPatternTest extends PredisCommandTestCase /** * @group connected - * @todo Disabled for now, must investigate why this test hangs on PUNSUBSCRIBE. */ - public function __testUnsubscribesFromAllSubscribedChannels() + public function testUnsubscribesFromAllSubscribedChannels() { $redis = $this->getClient(); $this->assertSame(array('subscribe', 'channel:foo', 1), $redis->subscribe('channel:foo')); $this->assertSame(array('subscribe', 'channel:bar', 2), $redis->subscribe('channel:bar')); - - $this->assertSame(array('punsubscribe', 'channel:foo', 1), $redis->punsubscribe()); - $this->assertSame(array('punsubscribe', 'channel:bar', 0), $redis->getConnection()->read()); - $this->assertSame('echoed', $redis->echo('echoed')); + $this->assertSame(array('punsubscribe', null, 2), $redis->punsubscribe()); } } diff --git a/tests/Predis/Command/ServerScriptTest.php b/tests/Predis/Command/ServerScriptTest.php index 4ab98fc9..88558073 100644 --- a/tests/Predis/Command/ServerScriptTest.php +++ b/tests/Predis/Command/ServerScriptTest.php @@ -57,7 +57,6 @@ class ServerScriptTest extends PredisCommandTestCase /** * @group connected - * @todo We should probably convert integers to booleans. */ public function testExistsReturnsAnArrayOfValues() { diff --git a/tests/Predis/Configuration/OptionsTest.php b/tests/Predis/Configuration/OptionsTest.php index c723255d..33343f73 100644 --- a/tests/Predis/Configuration/OptionsTest.php +++ b/tests/Predis/Configuration/OptionsTest.php @@ -15,8 +15,7 @@ use stdClass; use PredisTestCase; /** - * @todo We should test the inner work performed by this class - * using mock objects, but it is quite hard to to that. + * @todo Use mock objects to test the inner workings of the Options class. */ class OptionsTest extends PredisTestCase { diff --git a/tests/Predis/Connection/FactoryTest.php b/tests/Predis/Connection/FactoryTest.php index cb3138df..c368bfa0 100644 --- a/tests/Predis/Connection/FactoryTest.php +++ b/tests/Predis/Connection/FactoryTest.php @@ -137,6 +137,7 @@ class FactoryTest extends PredisTestCase /** * @group disconnected + * * @todo This test smells but there's no other way around it right now. */ public function testCreateConnectionWithInitializationCommands() diff --git a/tests/Predis/Monitor/ConsumerTest.php b/tests/Predis/Monitor/ConsumerTest.php index b14a5b3c..f300cee4 100644 --- a/tests/Predis/Monitor/ConsumerTest.php +++ b/tests/Predis/Monitor/ConsumerTest.php @@ -75,9 +75,10 @@ class ConsumerTest extends PredisTestCase /** * @group disconnected - * @todo We should investigate why disconnect is invoked 2 times in this test, - * but the reason is probably that the GC invokes __destruct() on monitor - * thus calling $client->disconnect() a second time at the end of the test. + * + * @todo Investigate why disconnect() is invoked 2 times in this test, but + * the reason is probably that the GC invokes __destruct() on monitor + * thus calling disconnect() a second time at the end of the test. */ public function testStoppingConsumerClosesConnection() { diff --git a/tests/Predis/Protocol/Text/ProtocolProcessorTest.php b/tests/Predis/Protocol/Text/ProtocolProcessorTest.php index d43045dc..ec0df5f1 100644 --- a/tests/Predis/Protocol/Text/ProtocolProcessorTest.php +++ b/tests/Predis/Protocol/Text/ProtocolProcessorTest.php @@ -47,8 +47,6 @@ class ProtocolProcessorTest extends PredisTestCase /** * @group disconnected - * - * @todo Improve test coverage */ public function testConnectionRead() {