jenkins-bot has submitted this change and it was merged.

Change subject: Message: Clean up unit tests and improve code coverage
......................................................................


Message: Clean up unit tests and improve code coverage

* Remove unnecessary use of ReflectionClass. It was testing
  internal properties that aren't part of the API. Using the
  getters instead.

* Remove need for func_get_args that was making the test more
  complex and the data provider hard to read. Simply maintain
  it as array of expected params and array of variadic arguments.

* Rename tests to more closely match tested methods.

* Rename data providers to provide*, and make them static.

* Reorder tests to more closely match logical order of the class.

* Improve line coverage from 31% to 67%.

Also:
* Remove testParams (dupes testConstructorParams).
* Add tests for RawMessage class.
* Add tests for transformation and parsing.
* Add tests for wfMessage().
* Add tests for Message::newFrom*.
* Add tests for "$*" replacement.
* Add tests for __toString.

Change-Id: I2b183a66f9e9f51bd800088e174b1ae4d3284d8d
---
M includes/Message.php
M tests/phpunit/includes/MessageTest.php
2 files changed, 244 insertions(+), 116 deletions(-)

Approvals:
  Nikerabbit: Looks good to me, approved
  jenkins-bot: Verified



diff --git a/includes/Message.php b/includes/Message.php
index 49437f4..134af0e 100644
--- a/includes/Message.php
+++ b/includes/Message.php
@@ -249,7 +249,7 @@
                $this->key = reset( $this->keysToTry );
 
                $this->parameters = array_values( $params );
-               $this->language = $language ? $language : $wgLang;
+               $this->language = $language ?: $wgLang;
        }
 
        /**
diff --git a/tests/phpunit/includes/MessageTest.php 
b/tests/phpunit/includes/MessageTest.php
index 4c5424c..99ec2e4 100644
--- a/tests/phpunit/includes/MessageTest.php
+++ b/tests/phpunit/includes/MessageTest.php
@@ -16,22 +16,11 @@
         * @dataProvider provideConstructor
         */
        public function testConstructor( $expectedLang, $key, $params, 
$language ) {
-               $reflection = new ReflectionClass( 'Message' );
-
-               $keyProperty = $reflection->getProperty( 'key' );
-               $keyProperty->setAccessible( true );
-
-               $paramsProperty = $reflection->getProperty( 'parameters' );
-               $paramsProperty->setAccessible( true );
-
-               $langProperty = $reflection->getProperty( 'language' );
-               $langProperty->setAccessible( true );
-
                $message = new Message( $key, $params, $language );
 
-               $this->assertEquals( $key, $keyProperty->getValue( $message ) );
-               $this->assertEquals( $params, $paramsProperty->getValue( 
$message ) );
-               $this->assertEquals( $expectedLang, $langProperty->getValue( 
$message ) );
+               $this->assertEquals( $key, $message->getKey() );
+               $this->assertEquals( $params, $message->getParams() );
+               $this->assertEquals( $expectedLang, $message->getLanguage() );
        }
 
        public static function provideConstructor() {
@@ -45,21 +34,62 @@
                );
        }
 
-       public static function provideTestParams() {
+       public static function provideConstructorParams() {
                return array(
-                       array( array() ),
-                       array( array( 'foo' ), 'foo' ),
-                       array( array( 'foo', 'bar' ), 'foo', 'bar' ),
-                       array( array( 'baz' ), array( 'baz' ) ),
-                       array( array( 'baz', 'foo' ), array( 'baz', 'foo' ) ),
-                       array( array( 'baz', 'foo' ), array( 'baz', 'foo' ), 
'hhh' ),
-                       array( array( 'baz', 'foo' ), array( 'baz', 'foo' ), 
'hhh', array( 'ahahahahha' ) ),
-                       array( array( 'baz', 'foo' ), array( 'baz', 'foo' ), 
array( 'ahahahahha' ) ),
-                       array( array( 'baz' ), array( 'baz' ), array( 
'ahahahahha' ) ),
+                       array(
+                               array(),
+                               array(),
+                       ),
+                       array(
+                               array( 'foo' ),
+                               array( 'foo' ),
+                       ),
+                       array(
+                               array( 'foo', 'bar' ),
+                               array( 'foo', 'bar' ),
+                       ),
+                       array(
+                               array( 'baz' ),
+                               array( array( 'baz' ) ),
+                       ),
+                       array(
+                               array( 'baz', 'foo' ),
+                               array( array( 'baz', 'foo' ) ),
+                       ),
+                       array(
+                               array( 'baz', 'foo' ),
+                               array( array( 'baz', 'foo' ), 'hhh' ),
+                       ),
+                       array(
+                               array( 'baz', 'foo' ),
+                               array( array( 'baz', 'foo' ), 'hhh', array( 
'ahahahahha' ) ),
+                       ),
+                       array(
+                               array( 'baz', 'foo' ),
+                               array( array( 'baz', 'foo' ), array( 
'ahahahahha' ) ),
+                       ),
+                       array(
+                               array( 'baz' ),
+                               array( array( 'baz' ), array( 'ahahahahha' ) ),
+                       ),
                );
        }
 
-       public function getLanguageProvider() {
+       /**
+        * @covers Message::__construct
+        * @covers Message::getParams
+        * @dataProvider provideConstructorParams
+        */
+       public function testConstructorParams( $expected, $args ) {
+               $msg = new Message( 'imasomething' );
+
+               $returned = call_user_func_array( array( $msg, 'params' ), 
$args );
+
+               $this->assertSame( $msg, $returned );
+               $this->assertEquals( $expected, $msg->getParams() );
+       }
+
+       public static function provideConstructorLanguage() {
                return array(
                        array( 'foo', array( 'bar' ), 'en' ),
                        array( 'foo', array( 'bar' ), 'de' )
@@ -67,27 +97,98 @@
        }
 
        /**
+        * @covers Message::__construct
         * @covers Message::getLanguage
-        * @dataProvider getLanguageProvider
+        * @dataProvider provideConstructorLanguage
         */
-       public function testGetLanguageCode( $key, $params, $languageCode ) {
+       public function testConstructorLanguage( $key, $params, $languageCode ) 
{
                $language = Language::factory( $languageCode );
                $message = new Message( $key, $params, $language );
 
                $this->assertEquals( $language, $message->getLanguage() );
        }
 
+       public static function provideKeys() {
+               return array(
+                       'string' => array(
+                               'key' => 'mainpage',
+                               'expected' => array( 'mainpage' ),
+                       ),
+                       'single' => array(
+                               'key' => array( 'mainpage' ),
+                               'expected' => array( 'mainpage' ),
+                       ),
+                       'multi' => array(
+                               'key' => array( 'mainpage-foo', 'mainpage-bar', 
'mainpage' ),
+                               'expected' => array( 'mainpage-foo', 
'mainpage-bar', 'mainpage' ),
+                       ),
+                       'empty' => array(
+                               'key' => array(),
+                               'expected' => null,
+                               'exception' => 'InvalidArgumentException',
+                       ),
+                       'null' => array(
+                               'key' => null,
+                               'expected' => null,
+                               'exception' => 'InvalidArgumentException',
+                       ),
+                       'bad type' => array(
+                               'key' => 123,
+                               'expected' => null,
+                               'exception' => 'InvalidArgumentException',
+                       ),
+               );
+       }
+
        /**
-        * @covers Message::params
-        * @dataProvider provideTestParams
+        * @covers Message::__construct
+        * @covers Message::getKey
+        * @covers Message::isMultiKey
+        * @covers Message::getKeysToTry
+        * @dataProvider provideKeys
         */
-       public function testParams( $expected ) {
-               $msg = new Message( 'imasomething' );
+       public function testKeys( $key, $expected, $exception = null ) {
+               if ( $exception ) {
+                       $this->setExpectedException( $exception );
+               }
 
-               $returned = call_user_func_array( array( $msg, 'params' ), 
array_slice( func_get_args(), 1 ) );
+               $msg = new Message( $key );
+               $this->assertContains( $msg->getKey(), $expected );
+               $this->assertEquals( $expected, $msg->getKeysToTry() );
+               $this->assertEquals( count( $expected ) > 1, $msg->isMultiKey() 
);
+       }
 
-               $this->assertSame( $msg, $returned );
-               $this->assertEquals( $expected, $msg->getParams() );
+       /**
+        * @covers ::wfMessage
+        */
+       public function testWfMessage() {
+               $this->assertInstanceOf( 'Message', wfMessage( 'mainpage' ) );
+               $this->assertInstanceOf( 'Message', wfMessage( 
'i-dont-exist-evar' ) );
+       }
+
+       /**
+        * @covers Message::newFromKey
+        */
+       public function testNewFromKey() {
+               $this->assertInstanceOf( 'Message', Message::newFromKey( 
'mainpage' ) );
+               $this->assertInstanceOf( 'Message', Message::newFromKey( 
'i-dont-exist-evar' ) );
+       }
+
+       /**
+        * @covers ::wfMessage
+        * @covers Message::__construct
+        */
+       public function testWfMessageParams() {
+               $this->assertEquals( 'Return to $1.', wfMessage( 'returnto' 
)->text() );
+               $this->assertEquals( 'Return to $1.', wfMessage( 'returnto', 
array() )->text() );
+               $this->assertEquals(
+                       'You have foo (bar).',
+                       wfMessage( 'youhavenewmessages', 'foo', 'bar' )->text()
+               );
+               $this->assertEquals(
+                       'You have foo (bar).',
+                       wfMessage( 'youhavenewmessages', array( 'foo', 'bar' ) 
)->text()
+               );
        }
 
        /**
@@ -104,10 +205,12 @@
 
        /**
         * @covers Message::__construct
+        * @covers Message::text
+        * @covers Message::plain
+        * @covers Message::escaped
+        * @covers Message::toString
         */
-       public function testKey() {
-               $this->assertInstanceOf( 'Message', wfMessage( 'mainpage' ) );
-               $this->assertInstanceOf( 'Message', wfMessage( 
'i-dont-exist-evar' ) );
+       public function testToStringKey() {
                $this->assertEquals( 'Main Page', wfMessage( 'mainpage' 
)->text() );
                $this->assertEquals( '<i-dont-exist-evar>', wfMessage( 
'i-dont-exist-evar' )->text() );
                $this->assertEquals( '<i<dont>exist-evar>', wfMessage( 
'i<dont>exist-evar' )->text() );
@@ -118,6 +221,26 @@
                        '&lt;i&lt;dont&gt;exist-evar&gt;',
                        wfMessage( 'i<dont>exist-evar' )->escaped()
                );
+       }
+
+       public static function provideToString() {
+               return array(
+                       array( 'mainpage', 'Main Page' ),
+                       array( 'i-dont-exist-evar', '<i-dont-exist-evar>' ),
+                       array( 'i-dont-exist-evar', 
'&lt;i-dont-exist-evar&gt;', 'escaped' ),
+               );
+       }
+
+       /**
+        * @covers Message::toString
+        * @covers Message::__toString
+        * @dataProvider provideToString
+        */
+       public function testToString( $key, $expect, $format = 'plain' ) {
+               $msg = new Message( $key );
+               $msg->$format();
+               $this->assertEquals( $expect, $msg->toString() );
+               $this->assertEquals( $expect, $msg->__toString() );
        }
 
        /**
@@ -138,26 +261,10 @@
        }
 
        /**
-        * @covers Message::__construct
-        */
-       public function testMessageParams() {
-               $this->assertEquals( 'Return to $1.', wfMessage( 'returnto' 
)->text() );
-               $this->assertEquals( 'Return to $1.', wfMessage( 'returnto', 
array() )->text() );
-               $this->assertEquals(
-                       'You have foo (bar).',
-                       wfMessage( 'youhavenewmessages', 'foo', 'bar' )->text()
-               );
-               $this->assertEquals(
-                       'You have foo (bar).',
-                       wfMessage( 'youhavenewmessages', array( 'foo', 'bar' ) 
)->text()
-               );
-       }
-
-       /**
-        * @covers Message::__construct
+        * @covers Message::rawParam
         * @covers Message::rawParams
         */
-       public function testMessageParamSubstitution() {
+       public function testRawParams() {
                $this->assertEquals(
                        '(Заглавная страница)',
                        wfMessage( 'parentheses', 'Заглавная страница' 
)->plain()
@@ -177,10 +284,21 @@
        }
 
        /**
-        * @covers Message::__construct
-        * @covers Message::params
+        * @covers RawMessage::__construct
+        * @covers RawMessage::fetchMessage
         */
-       public function testDeliciouslyManyParams() {
+       public function testRawMessage() {
+               $msg = new RawMessage( 'example &' );
+               $this->assertEquals( 'example &', $msg->plain() );
+               $this->assertEquals( 'example &amp;', $msg->escaped() );
+       }
+
+       /**
+        * @covers Message::params
+        * @covers Message::toString
+        * @covers Message::replaceParameters
+        */
+       public function testReplaceManyParams() {
                $msg = new RawMessage( '$1$2$3$4$5$6$7$8$9$10$11$12' );
                // One less than above has placeholders
                $params = array( 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 
'j', 'k' );
@@ -189,12 +307,20 @@
                        $msg->params( $params )->plain(),
                        'Params > 9 are replaced correctly'
                );
+
+               $msg = new RawMessage( 'Params$*' );
+               $params = array( 'ab', 'bc', 'cd' );
+               $this->assertEquals(
+                       'Params: ab, bc, cd',
+                       $msg->params( $params )->text()
+               );
        }
 
        /**
+        * @covers Message::numParam
         * @covers Message::numParams
         */
-       public function testMessageNumParams() {
+       public function testNumParams() {
                $lang = Language::factory( 'en' );
                $msg = new RawMessage( '$1' );
 
@@ -206,9 +332,10 @@
        }
 
        /**
+        * @covers Message::durationParam
         * @covers Message::durationParams
         */
-       public function testMessageDurationParams() {
+       public function testDurationParams() {
                $lang = Language::factory( 'en' );
                $msg = new RawMessage( '$1' );
 
@@ -222,9 +349,10 @@
        /**
         * FIXME: This should not need database, but Language#formatExpiry does 
(bug 55912)
         * @group Database
+        * @covers Message::expiryParam
         * @covers Message::expiryParams
         */
-       public function testMessageExpiryParams() {
+       public function testExpiryParams() {
                $lang = Language::factory( 'en' );
                $msg = new RawMessage( '$1' );
 
@@ -236,9 +364,10 @@
        }
 
        /**
+        * @covers Message::timeperiodParam
         * @covers Message::timeperiodParams
         */
-       public function testMessageTimeperiodParams() {
+       public function testTimeperiodParams() {
                $lang = Language::factory( 'en' );
                $msg = new RawMessage( '$1' );
 
@@ -250,9 +379,10 @@
        }
 
        /**
+        * @covers Message::sizeParam
         * @covers Message::sizeParams
         */
-       public function testMessageSizeParams() {
+       public function testSizeParams() {
                $lang = Language::factory( 'en' );
                $msg = new RawMessage( '$1' );
 
@@ -264,9 +394,10 @@
        }
 
        /**
+        * @covers Message::bitrateParam
         * @covers Message::bitrateParams
         */
-       public function testMessageBitrateParams() {
+       public function testBitrateParams() {
                $lang = Language::factory( 'en' );
                $msg = new RawMessage( '$1' );
 
@@ -277,7 +408,7 @@
                );
        }
 
-       public function messagePlaintextParamsProvider() {
+       public static function providePlaintextParams() {
                return array(
                        array(
                                'one $2 <div>foo</div> [[Bar]] {{Baz}} &lt;',
@@ -308,10 +439,15 @@
        }
 
        /**
-        * @dataProvider messagePlaintextParamsProvider
+        * @covers Message::plaintextParam
         * @covers Message::plaintextParams
+        * @covers Message::formatPlaintext
+        * @covers Message::toString
+        * @covers Message::parse
+        * @covers Message::parseAsBlock
+        * @dataProvider providePlaintextParams
         */
-       public function testMessagePlaintextParams( $expect, $format ) {
+       public function testPlaintextParams( $expect, $format ) {
                $lang = Language::factory( 'en' );
 
                $msg = new RawMessage( '$1 $2' );
@@ -323,6 +459,46 @@
                        $expect,
                        $msg->inLanguage( $lang )->plaintextParams( $params 
)->$format(),
                        "Fail formatting for $format"
+               );
+       }
+
+       public static function provideParser() {
+               return array(
+                       array(
+                               "''&'' <x><!-- x -->",
+                               'plain',
+                       ),
+
+                       array(
+                               "''&'' <x><!-- x -->",
+                               'text',
+                       ),
+                       array(
+                               '<i>&amp;</i> &lt;x&gt;',
+                               'parse',
+                       ),
+
+                       array(
+                               "<p><i>&amp;</i> &lt;x&gt;\n</p>",
+                               'parseAsBlock',
+                       ),
+               );
+       }
+
+       /**
+        * @covers Message::text
+        * @covers Message::parse
+        * @covers Message::parseAsBlock
+        * @covers Message::toString
+        * @covers Message::transformText
+        * @covers Message::parseText
+        * @dataProvider provideParser
+        */
+       public function testParser( $expect, $format ) {
+               $msg = new RawMessage( "''&'' <x><!-- x -->" );
+               $this->assertEquals(
+                       $expect,
+                       $msg->inLanguage( 'en' )->$format()
                );
        }
 
@@ -371,53 +547,5 @@
         */
        public function testInLanguageThrows() {
                wfMessage( 'foo' )->inLanguage( 123 );
-       }
-
-       public function keyProvider() {
-               return array(
-                       'string' => array(
-                               'key' => 'mainpage',
-                               'expected' => array( 'mainpage' ),
-                       ),
-                       'single' => array(
-                               'key' => array( 'mainpage' ),
-                               'expected' => array( 'mainpage' ),
-                       ),
-                       'multi' => array(
-                               'key' => array( 'mainpage-foo', 'mainpage-bar', 
'mainpage' ),
-                               'expected' => array( 'mainpage-foo', 
'mainpage-bar', 'mainpage' ),
-                       ),
-                       'empty' => array(
-                               'key' => array(),
-                               'expected' => null,
-                               'exception' => 'InvalidArgumentException',
-                       ),
-                       'null' => array(
-                               'key' => null,
-                               'expected' => null,
-                               'exception' => 'InvalidArgumentException',
-                       ),
-                       'bad type' => array(
-                               'key' => 17,
-                               'expected' => null,
-                               'exception' => 'InvalidArgumentException',
-                       ),
-               );
-       }
-
-       /**
-        * @dataProvider keyProvider()
-        *
-        * @covers Message::getKey
-        */
-       public function testGetKey( $key, $expected, $exception = null ) {
-               if ( $exception ) {
-                       $this->setExpectedException( $exception );
-               }
-
-               $msg = new Message( $key );
-               $this->assertEquals( $expected, $msg->getKeysToTry() );
-               $this->assertEquals( count( $expected ) > 1, $msg->isMultiKey() 
);
-               $this->assertContains( $msg->getKey(), $expected );
        }
 }

-- 
To view, visit https://gerrit.wikimedia.org/r/201422
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I2b183a66f9e9f51bd800088e174b1ae4d3284d8d
Gerrit-PatchSet: 3
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Krinkle <krinklem...@gmail.com>
Gerrit-Reviewer: Aude <aude.w...@gmail.com>
Gerrit-Reviewer: Krinkle <krinklem...@gmail.com>
Gerrit-Reviewer: Nikerabbit <niklas.laxst...@gmail.com>
Gerrit-Reviewer: jenkins-bot <>

_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to