Add unit tests for new Metrics Formatter classes and remove existing ones. Small change to Overlay included and bug fixes to metrics formatting code included.

5 files changed, 342 insertions, 101 deletions

diff --git a/core/Metrics/Formatter.php b/core/Metrics/Formatter.php
index ac3c6f0278..44103d7e27 100644
--- a/core/Metrics/Formatter.php
+++ b/core/Metrics/Formatter.php
@@ -23,6 +23,9 @@ use Piwik\Tracker\GoalManager;
  */
 class Formatter
 {
+    private $decimalPoint = null;
+    private $thousandsSeparator = null;
+
     /**
      * Returns a prettified string representation of a number. The result will have
      * thousands separators and a decimal point specific to the current locale, eg,
@@ -31,19 +34,16 @@ class Formatter
      * @param number $value
      * @return string
      */
-    public function getPrettyNumber($value)
+    public function getPrettyNumber($value, $precision = 0)
     {
-        static $decimalPoint = null;
-        static $thousandsSeparator = null;
-
-        if ($decimalPoint === null) {
+        if ($this->decimalPoint === null) {
             $locale = localeconv();
 
-            $decimalPoint = $locale['decimal_point'];
-            $thousandsSeparator = $locale['thousands_sep'];
+            $this->decimalPoint = $locale['decimal_point'];
+            $this->thousandsSeparator = $locale['thousands_sep'];
         }
 
-        return number_format($value, 0, $decimalPoint, $thousandsSeparator);
+        return number_format($value, $precision, $this->decimalPoint, $this->thousandsSeparator);
     }
 
     /**
@@ -51,7 +51,6 @@ class Formatter
      *
      * @param int $numberOfSeconds The number of seconds.
      * @param bool $displayTimeAsSentence If set to true, will output `"5min 17s"`, if false `"00:05:17"`.
-     * @param bool $isHtml If true, replaces all spaces with `' '`.
      * @param bool $round Whether to round to the nearest second or not.
      * @return string
      */
@@ -132,8 +131,8 @@ class Formatter
         $units = array('B', 'K', 'M', 'G', 'T');
 
         $currentUnit = null;
-        foreach ($units as $currentUnit) {
-            if ($size >= 1024 && $unit != $currentUnit) {
+        foreach ($units as $idx => $currentUnit) {
+            if ($size >= 1024 && $unit != $currentUnit && $idx != count($units) - 1) {
                 $size = $size / 1024;
             } else {
                 break;
@@ -148,22 +147,23 @@ class Formatter
      *
      * @param int|string $value The monetary value to format.
      * @param int $idSite The ID of the site whose currency will be used.
-     * @param bool $isHtml If true, replaces all spaces with `' '`.
      * @return string
      */
     public function getPrettyMoney($value, $idSite)
     {
-        $currencyBefore = self::getCurrencySymbol($idSite);
-
         $space = ' ';
 
+        $currencySymbol = self::getCurrencySymbol($idSite);
+
+        $currencyBefore =  $currencySymbol . $space;
         $currencyAfter = '';
+
         // (maybe more currencies prefer this notation?)
         $currencySymbolToAppend = array('€', 'kr', 'zł');
 
         // manually put the currency symbol after the amount
-        if (in_array($currencyBefore, $currencySymbolToAppend)) {
-            $currencyAfter = $space . $currencyBefore;
+        if (in_array($currencySymbol, $currencySymbolToAppend)) {
+            $currencyAfter = $space . $currencySymbol;
             $currencyBefore = '';
         }
 
@@ -179,7 +179,7 @@ class Formatter
             }
         }
 
-        $prettyMoney = $currencyBefore . $space . $value . $currencyAfter;
+        $prettyMoney = $currencyBefore . $value . $currencyAfter;
         return $prettyMoney;
     }
 
diff --git a/plugins/Overlay/Controller.php b/plugins/Overlay/Controller.php
index 603047c286..d55ec7e6e9 100644
--- a/plugins/Overlay/Controller.php
+++ b/plugins/Overlay/Controller.php
@@ -76,7 +76,8 @@ class Controller extends \Piwik\Plugin\Controller
         );
         $dataTable = $request->process();
 
-        $formatter = new Metrics\Formatter();
+        // TODO: move metric formatting logic to Formatter\Html from DataTablePostProcessor
+        $formatter = new Metrics\Formatter\Html();
 
         $data = array();
         if ($dataTable->getRowsCount() > 0) {
diff --git a/tests/PHPUnit/Integration/PiwikTest.php b/tests/PHPUnit/Integration/PiwikTest.php
index 252c08cd78..6043736e04 100644
--- a/tests/PHPUnit/Integration/PiwikTest.php
+++ b/tests/PHPUnit/Integration/PiwikTest.php
@@ -9,8 +9,6 @@
 namespace Piwik\Tests\Integration;
 
 use Piwik\Access;
-use Piwik\Filesystem;
-use Piwik\MetricsFormatter;
 use Piwik\Piwik;
 use Piwik\Plugins\SitesManager\API;
 use Piwik\Translate;
@@ -80,52 +78,6 @@ class PiwikTest extends IntegrationTestCase
     }
 
     /**
-     * Dataprovider for testGetPrettyTimeFromSeconds
-     */
-    public function getPrettyTimeFromSecondsData()
-    {
-        return array(
-            array(30, array('30s', '00:00:30')),
-            array(60, array('1 min 0s', '00:01:00')),
-            array(100, array('1 min 40s', '00:01:40')),
-            array(3600, array('1 hours 0 min', '01:00:00')),
-            array(3700, array('1 hours 1 min', '01:01:40')),
-            array(86400 + 3600 * 10, array('1 days 10 hours', '34:00:00')),
-            array(86400 * 365, array('365 days 0 hours', '8760:00:00')),
-            array((86400 * (365.25 + 10)), array('1 years 10 days', '9006:00:00')),
-            array(1.342, array('1.34s', '00:00:01.34')),
-            array(.342, array('0.34s', '00:00:00.34')),
-            array(.02, array('0.02s', '00:00:00.02')),
-            array(.002, array('0.002s', '00:00:00')),
-            array(1.002, array('1s', '00:00:01')),
-            array(1.02, array('1.02s', '00:00:01.02')),
-            array(1.2, array('1.2s', '00:00:01.20')),
-            array(122.1, array('2 min 2.1s', '00:02:02.10')),
-            array(-122.1, array('-2 min 2.1s', '-00:02:02.10')),
-            array(86400 * -365, array('-365 days 0 hours', '-8760:00:00'))
-        );
-    }
-
-    /**
-     * @dataProvider getPrettyTimeFromSecondsData
-     */
-    public function testGetPrettyTimeFromSeconds($seconds, $expected)
-    {
-        if (($seconds * 100) > PHP_INT_MAX) {
-            $this->markTestSkipped("Will not pass on 32-bit machine.");
-        }
-
-        Translate::loadEnglishTranslation();
-
-        $sentenceExpected = str_replace(' ', ' ', $expected[0]);
-        $numericExpected = $expected[1];
-        $this->assertEquals($sentenceExpected, MetricsFormatter::getPrettyTimeFromSeconds($seconds, $sentence = true));
-        $this->assertEquals($numericExpected, MetricsFormatter::getPrettyTimeFromSeconds($seconds, $sentence = false));
-
-        Translate::unloadEnglishTranslation();
-    }
-
-    /**
      * Dataprovider for testCheckValidLoginString
      */
     public function getInvalidLoginStringData()
@@ -184,41 +136,6 @@ class PiwikTest extends IntegrationTestCase
     }
 
     /**
-     * Dataprovider for testGetPrettyValue
-     */
-    public function getGetPrettyValueTestCases()
-    {
-        return array(
-            array('revenue', 12, '$ 12'),
-            array('revenue_evolution', '100 %', '100 %'),
-            array('avg_time_generation', '3.333', '3.33s'),
-            array('avg_time_generation', '333.333', '5 min 33.33s'),
-            array('avg_time_on_page', '3', '00:00:03'),
-            array('avg_time_on_page', '333', '00:05:33'),
-        );
-    }
-
-    /**
-     * @dataProvider getGetPrettyValueTestCases
-     */
-    public function testGetPrettyValue($columnName, $value, $expected)
-    {
-        Translate::loadEnglishTranslation();
-
-        $access = Access::getInstance();
-        $access->setSuperUserAccess(true);
-
-        $idsite = API::getInstance()->addSite("test", "http://test");
-
-        $this->assertEquals(
-            $expected,
-            MetricsFormatter::getPrettyValue($idsite, $columnName, $value, false, false)
-        );
-
-        Translate::unloadEnglishTranslation();
-    }
-
-    /**
      * Data provider for testIsAssociativeArray.
      */
     public function getIsAssociativeArrayTestCases()
diff --git a/tests/PHPUnit/Unit/Metrics/Formatter/HtmlTest.php b/tests/PHPUnit/Unit/Metrics/Formatter/HtmlTest.php
new file mode 100644
index 0000000000..fd9b996f50
--- /dev/null
+++ b/tests/PHPUnit/Unit/Metrics/Formatter/HtmlTest.php
@@ -0,0 +1,91 @@
+<?php
+/**
+ * Piwik - free/libre analytics platform
+ *
+ * @link http://piwik.org
+ * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
+ */
+namespace Piwik\Tests\Unit\Metrics\Formatter;
+
+use Piwik\Metrics\Formatter\Html;
+use Piwik\Translate;
+use Piwik\Plugins\SitesManager\API as SitesManagerAPI;
+
+/**
+ * @group Core
+ */
+class HtmlTest extends \PHPUnit_Framework_TestCase
+{
+    /**
+     * @var Html
+     */
+    private $formatter;
+
+    private $sitesInfo;
+
+    public function setUp()
+    {
+        $this->sitesInfo = array(
+            1 => array(
+                'idsite' => '1',
+                'currency' => 'EUR'
+            )
+        );
+
+        $this->formatter = new Html();
+
+        setlocale(LC_ALL, null);
+
+        Translate::loadEnglishTranslation();
+        $this->setSiteManagerApiMock();
+    }
+
+    public function tearDown()
+    {
+        Translate::unloadEnglishTranslation();
+        $this->unsetSiteManagerApiMock();
+
+        setlocale(LC_ALL, null);
+    }
+
+    public function test_getPrettyTimeFromSeconds_DefaultsToShowingSentences_AndUsesNonBreakingSpaces()
+    {
+        $expected = '1&nbsp;days&nbsp;10&nbsp;hours';
+        $value = $this->formatter->getPrettyTimeFromSeconds(86400 + 3600 * 10);
+
+        $this->assertEquals($expected, $value);
+    }
+
+    public function test_getPrettySizeFromBytes_UsesNonBreakingSpaces()
+    {
+        $expected = '1.5&nbsp;K';
+        $value = $this->formatter->getPrettySizeFromBytes(1536);
+
+        $this->assertEquals($expected, $value);
+    }
+
+    public function test_getPrettyMoney_UsesNonBreakingSpaces()
+    {
+        $expected = '1&nbsp;€';
+        $value = $this->formatter->getPrettyMoney(1, 1);
+
+        $this->assertEquals($expected, $value);
+    }
+
+    private function unsetSiteManagerApiMock()
+    {
+        SitesManagerAPI::unsetInstance();
+    }
+
+    private function setSiteManagerApiMock()
+    {
+        $sitesInfo = $this->sitesInfo;
+
+        $mock = $this->getMock('stdClass', array('getSiteFromId'));
+        $mock->expects($this->any())->method('getSiteFromId')->willReturnCallback(function ($idSite) use ($sitesInfo) {
+            return $sitesInfo[$idSite];
+        });
+
+        SitesManagerAPI::setSingletonInstance($mock);
+    }
+}
\ No newline at end of file
diff --git a/tests/PHPUnit/Unit/Metrics/FormatterTest.php b/tests/PHPUnit/Unit/Metrics/FormatterTest.php
new file mode 100644
index 0000000000..8ef4831299
--- /dev/null
+++ b/tests/PHPUnit/Unit/Metrics/FormatterTest.php
@@ -0,0 +1,232 @@
+<?php
+/**
+ * Piwik - free/libre analytics platform
+ *
+ * @link http://piwik.org
+ * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
+ */
+namespace Piwik\Tests\Unit\Metrics;
+
+use Piwik\Metrics\Formatter;
+use Piwik\Translate;
+use Piwik\Plugins\SitesManager\API as SitesManagerAPI;
+
+/**
+ * @group Core
+ */
+class FormatterTest extends \PHPUnit_Framework_TestCase
+{
+    /**
+     * @var Formatter
+     */
+    private $formatter;
+
+    private $sitesInfo;
+
+    public function setUp()
+    {
+        $this->sitesInfo = array(
+            1 => array(
+                'idsite' => '1',
+                'currency' => 'EUR'
+            ),
+            2 => array(
+                'idsite' => '2',
+                'currency' => 'DKK'
+            ),
+            3 => array(
+                'idsite' => '3',
+                'currency' => 'PLN'
+            ),
+            4 => array(
+                'idsite' => '4',
+                'currency' => 'NZD'
+            ),
+            5 => array(
+                'idsite' => '5',
+                'currency' => 'JPY'
+            )
+        );
+
+        $this->formatter = new Formatter();
+
+        setlocale(LC_ALL, null);
+
+        Translate::loadEnglishTranslation();
+        $this->setSiteManagerApiMock();
+    }
+
+    public function tearDown()
+    {
+        Translate::unloadEnglishTranslation();
+        $this->unsetSiteManagerApiMock();
+
+        setlocale(LC_ALL, null);
+    }
+
+    /**
+     * @dataProvider getPrettyNumberTestData
+     */
+    public function test_getPrettyNumber_ReturnsCorrectResult($number, $expected)
+    {
+        $this->assertEquals($expected, $this->formatter->getPrettyNumber($number, 2));
+    }
+
+    /**
+     * @dataProvider getPrettyNumberLocaleTestData
+     */
+    public function test_getPrettyNumber_ReturnsCorrectResult_WhenLocaleIsEuropean($number, $expected)
+    {
+        $locale = setlocale(LC_ALL, array('de', 'de_DE', 'ge', 'de_DE.utf8'));
+        if (empty($locale)) {
+            $this->markTestSkipped("de_DE locale is not present on this system");
+        }
+
+        $this->assertEquals($expected, $this->formatter->getPrettyNumber($number, 2));
+    }
+
+    /**
+     * @dataProvider getPrettySizeFromBytesTestData
+     */
+    public function test_getPrettySizeFromBytes_ReturnsCorrectResult($bytesSize, $unit, $expected)
+    {
+        $this->assertEquals($expected, $this->formatter->getPrettySizeFromBytes($bytesSize, $unit));
+    }
+
+    /**
+     * @dataProvider getPrettyMoneyTestData
+     */
+    public function test_getPrettyMoney_ReturnsCorrectResult($value, $idSite, $expected)
+    {
+        $this->assertEquals($expected, $this->formatter->getPrettyMoney($value, $idSite));
+    }
+
+    /**
+     * @dataProvider getPrettyPercentFromQuotientTestData
+     */
+    public function test_getPrettyPercentFromQuotient_ReturnsCorrectResult($value, $expected)
+    {
+        $this->assertEquals($expected, $this->formatter->getPrettyPercentFromQuotient($value));
+    }
+
+    /**
+     * @dataProvider getPrettyTimeFromSecondsData
+     */
+    public function test_getPrettyTimeFromSeconds_ReturnsCorrectResult($seconds, $expected)
+    {
+        if (($seconds * 100) > PHP_INT_MAX) {
+            $this->markTestSkipped("Will not pass on 32-bit machine.");
+        }
+
+        $sentenceExpected = $expected[0];
+        $this->assertEquals($sentenceExpected, $this->formatter->getPrettyTimeFromSeconds($seconds, $sentence = true));
+
+        $numericExpected = $expected[1];
+        $this->assertEquals($numericExpected, $this->formatter->getPrettyTimeFromSeconds($seconds, $sentence = false));
+    }
+
+    public function getPrettyNumberTestData()
+    {
+        return array(
+            array(0.14, '0.14'),
+            array(0.14567, '0.15'),
+            array(100.1234, '100.12'),
+            array(1000.45, '1,000.45'),
+            array(23456789.00, '23,456,789.00')
+        );
+    }
+
+    public function getPrettyNumberLocaleTestData()
+    {
+        return array(
+            array(0.14, '0,14'),
+            array(0.14567, '0,15'),
+            array(100.1234, '100,12'),
+            array(1000.45, '1.000,45'),
+            array(23456789.00, '23.456.789,00')
+        );
+    }
+
+    public function getPrettySizeFromBytesTestData()
+    {
+        return array(
+            array(767, null, '767 B'),
+            array(1024, null, '1 K'),
+            array(1536, null, '1.5 K'),
+            array(1024 * 1024, null, '1 M'),
+            array(1.25 * 1024 * 1024, null, '1.3 M'),
+            array(1.25 * 1024 * 1024 * 1024, null, '1.3 G'),
+            array(1.25 * 1024 * 1024 * 1024 * 1024, null, '1.3 T'),
+            array(1.25 * 1024 * 1024 * 1024 * 1024 * 1024, null, '1280 T'),
+            array(1.25 * 1024 * 1024, 'M', '1.3 M'),
+            array(1.25 * 1024 * 1024 * 1024, 'M', '1280 M'),
+            array(0, null, '0 M')
+        );
+    }
+
+    public function getPrettyMoneyTestData()
+    {
+        return array(
+            array(1, 1, '1 €'),
+            array(1.045, 2, '1.04 kr'),
+            array(1000.4445, 3, '1000.44 zł'),
+            array(1234.56, 4, '$ 1234.56'),
+            array(234.76, 5, '¥ 234.76')
+        );
+    }
+
+    public function getPrettyPercentFromQuotientTestData()
+    {
+        return array(
+            array(100, '10000%'),
+            array(1, '100%'),
+            array(.85, '85%'),
+            array(.89999, '89.999%'),
+            array(.0004, '0.04%')
+        );
+    }
+
+    /**
+     * Dataprovider for testGetPrettyTimeFromSeconds
+     */
+    public function getPrettyTimeFromSecondsData()
+    {
+        return array(
+            array(30, array('30s', '00:00:30')),
+            array(60, array('1 min 0s', '00:01:00')),
+            array(100, array('1 min 40s', '00:01:40')),
+            array(3600, array('1 hours 0 min', '01:00:00')),
+            array(3700, array('1 hours 1 min', '01:01:40')),
+            array(86400 + 3600 * 10, array('1 days 10 hours', '34:00:00')),
+            array(86400 * 365, array('365 days 0 hours', '8760:00:00')),
+            array((86400 * (365.25 + 10)), array('1 years 10 days', '9006:00:00')),
+            array(1.342, array('1.34s', '00:00:01.34')),
+            array(.342, array('0.34s', '00:00:00.34')),
+            array(.02, array('0.02s', '00:00:00.02')),
+            array(.002, array('0.002s', '00:00:00')),
+            array(1.002, array('1s', '00:00:01')),
+            array(1.02, array('1.02s', '00:00:01.02')),
+            array(1.2, array('1.2s', '00:00:01.20')),
+            array(122.1, array('2 min 2.1s', '00:02:02.10')),
+            array(-122.1, array('-2 min 2.1s', '-00:02:02.10')),
+            array(86400 * -365, array('-365 days 0 hours', '-8760:00:00'))
+        );
+    }
+
+    private function unsetSiteManagerApiMock()
+    {
+        SitesManagerAPI::unsetInstance();
+    }
+
+    private function setSiteManagerApiMock()
+    {
+        $sitesInfo = $this->sitesInfo;
+
+        $mock = $this->getMock('stdClass', array('getSiteFromId'));
+        $mock->expects($this->any())->method('getSiteFromId')->willReturnCallback(function ($idSite) use ($sitesInfo) {
+            return $sitesInfo[$idSite];
+        });
+
+        SitesManagerAPI::setSingletonInstance($mock);
+    }
+}
\ No newline at end of file