[tests] Apply some fixes and improvements and remove old stuff.

phpunit.xml.dist

@@ -1,6 +1,11 @@
 <?xml version="1.0" encoding="UTF-8"?>
 
-<phpunit bootstrap="tests/bootstrap.php" colors="true" strict="true">
+<phpunit bootstrap="tests/bootstrap.php"
+    colors="true"
+    beStrictAboutTestSize="true"
+    checkForUnintentionallyCoveredCode="true"
+    beStrictAboutTestsThatDoNotTestAnything="true">
+
     <testsuites>
         <testsuite name="Predis Test Suite">
             <directory>tests/Predis/</directory>

phpunit.xml.travisci

@@ -1,6 +1,11 @@
 <?xml version="1.0" encoding="UTF-8"?>
 
-<phpunit bootstrap="tests/bootstrap.php" colors="true" strict="true">
+<phpunit bootstrap="tests/bootstrap.php"
+    colors="true"
+    beStrictAboutTestSize="true"
+    checkForUnintentionallyCoveredCode="true"
+    beStrictAboutTestsThatDoNotTestAnything="true">
+
     <testsuites>
         <testsuite name="Predis Test Suite">
             <directory>tests/Predis/</directory>

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');

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.
-     *
-     * @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.
+     * 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 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."
             );

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');

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
 {

tests/Predis/Cluster/Distributor/HashRingTest.php

@@ -12,7 +12,7 @@
 namespace Predis\Cluster\Distributor;
 
 /**
- * @todo To be improved.
+ *
  */
 class HashRingTest extends PredisDistributorTestCase
 {

tests/Predis/Cluster/Distributor/KetamaRingTest.php

@@ -12,7 +12,7 @@
 namespace Predis\Cluster\Distributor;
 
 /**
- * @todo To be improved.
+ *
  */
 class KetamaRingTest extends PredisDistributorTestCase
 {

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()
     {

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
 {

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
 {

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
 {

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()
     {

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'));
+        }
     }
 }

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'));
+        }
     }
 }

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
 {

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
 {

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
 {

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());
     }
 }

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()
     {

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
 {

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()

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()
     {

tests/Predis/Protocol/Text/ProtocolProcessorTest.php

@@ -47,8 +47,6 @@ class ProtocolProcessorTest extends PredisTestCase
 
     /**
      * @group disconnected
-     *
-     * @todo Improve test coverage
      */
     public function testConnectionRead()
     {