JavaScriptMinifier: Add better line-breaker tests

Set maxLineLength to '1' instead of messing with filler content.
This makes it easy to see all potential line-breaks, instead of only
at the 999th offset.

Bug: T201606
Change-Id: I220b145c5bc8e7d1a41efacd2a6cea738545f006

diff --git a/includes/libs/JavaScriptMinifier.php b/includes/libs/JavaScriptMinifier.php
index aaf62b5..8541c4f 100644 (file)
--- a/includes/libs/JavaScriptMinifier.php
+++ b/includes/libs/JavaScriptMinifier.php
@@ -73,11 +73,16 @@ class JavaScriptMinifier {
        const STACK_LIMIT = 1000;
 
        /**
-        * NOTE: This isn't a strict maximum. Longer lines will be produced when
-        *       literals (e.g. quoted strings) longer than this are encountered
-        *       or when required to guard against semicolon insertion.
+        * Maximum line length
+        *
+        * This is not a strict maximum, but a guideline. Longer lines will be
+        * produced when literals (e.g. quoted strings) longer than this are
+        * encountered, or when required to guard against semicolon insertion.
+        *
+        * This is a private member (instead of constant) to allow tests to
+        * set it to 1, to verify ASI and line-breaking behaviour.
         */
-       const MAX_LINE_LENGTH = 1000;
+       private static $maxLineLength = 1000;
 
        /**
         * Returns minified JavaScript code.
@@ -672,7 +677,7 @@ class JavaScriptMinifier {
                                $out .= "\n";
                                $state = self::STATEMENT;
                                $lineLength = 0;
-                       } elseif ( $lineLength + $end - $pos > self::MAX_LINE_LENGTH &&
+                       } elseif ( $lineLength + $end - $pos > self::$maxLineLength &&
                                        !isset( $semicolon[$state][$type] ) && $type !== self::TYPE_INCR_OP ) {
                                // This line would get too long if we added $token, so add a newline first.
                                // Only do this if it won't trigger semicolon insertion and if it won't

diff --git a/tests/phpunit/includes/libs/JavaScriptMinifierTest.php b/tests/phpunit/includes/libs/JavaScriptMinifierTest.php
index 03a4438..70f9c7c 100644 (file)
--- a/tests/phpunit/includes/libs/JavaScriptMinifierTest.php
+++ b/tests/phpunit/includes/libs/JavaScriptMinifierTest.php
@@ -4,6 +4,19 @@ class JavaScriptMinifierTest extends PHPUnit\Framework\TestCase {
 
        use MediaWikiCoversValidator;
 
+       protected function tearDown() {
+               parent::tearDown();
+               // Reset
+               $this->setMaxLineLength( 1000 );
+       }
+
+       private function setMaxLineLength( $val ) {
+               $classReflect = new ReflectionClass( JavaScriptMinifier::class );
+               $propertyReflect = $classReflect->getProperty( 'maxLineLength' );
+               $propertyReflect->setAccessible( true );
+               $propertyReflect->setValue( JavaScriptMinifier::class, $val );
+       }
+
        public static function provideCases() {
                return [
 
@@ -203,67 +216,106 @@ class JavaScriptMinifierTest extends PHPUnit\Framework\TestCase {
                );
        }
 
-       /**
-        * @covers JavaScriptMinifier::minify
-        */
-       public function testReturnLineBreak() {
-               // Regression test for T201606.
-               $lineFill = str_repeat( 'x', 993 );
-               $code = <<<JAVASCRIPT
-call( function () {
-       try {
-       } catch (e) {
-               push = {
-                       apply: 1 ? 0 : {}
-               };
-       }
-       {$lineFill}
-       return name === 'input';
-} );
-JAVASCRIPT;
-               $this->assertSame(
-                       "call(function(){try{}catch(e){push={apply:1?0:{}};}"
-                               // FIXME: Token `name` must be on line 2 instead of line 3
-                               . "\n$lineFill return"
-                               . "\nname==='input';});",
-                       JavaScriptMinifier::minify( $code )
-               );
-       }
-
-       public static function provideExponentLineBreaking() {
+       public static function provideLineBreaker() {
                return [
                        [
-                               // This one gets interpreted all together by the prior code;
-                               // no break at the 'E' happens.
-                               '1.23456789E55',
+                               // Regression tests for T34548.
+                               // Must not break between 'E' and '+'.
+                               'var name = 1.23456789E55;',
+                               [
+                                       'var',
+                                       'name',
+                                       '=',
+                                       '1.23456789E55',
+                                       ';',
+                               ],
                        ],
                        [
-                               // This one breaks under the bad code; splits between 'E' and '+'
-                               '1.23456789E+5',
+                               'var name = 1.23456789E+5;',
+                               [
+                                       'var',
+                                       'name',
+                                       '=',
+                                       '1.23456789E+5',
+                                       ';',
+                               ],
                        ],
                        [
-                               // This one breaks under the bad code; splits between 'E' and '-'
-                               '1.23456789E-5',
+                               'var name = 1.23456789E-5;',
+                               [
+                                       'var',
+                                       'name',
+                                       '=',
+                                       '1.23456789E-5',
+                                       ';',
+                               ],
                        ],
+                       [
+                               // Regression test for T201606.
+                               // Must not break between 'return' and Expression.
+                               <<<JAVASCRIPT
+                       call( function () {
+                               try {
+                               } catch (e) {
+                                       push = {
+                                               apply: 1 ? 0 : {}
+                                       };
+                               }
+                               return name === 'input';
+                       } );
+JAVASCRIPT
+                               ,
+                               [
+                                       'call',
+                                       '(',
+                                       'function',
+                                       '(',
+                                       ')',
+                                       '{',
+                                       'try',
+                                       '{',
+                                       '}',
+                                       'catch',
+                                       '(',
+                                       'e',
+                                       ')',
+                                       '{',
+                                       'push',
+                                       '=',
+                                       '{',
+                                       'apply',
+                                       ':',
+                                       '1',
+                                       '?',
+                                       '0',
+                                       ':',
+                                       '{',
+                                       '}',
+                                       '}',
+                                       ';',
+                                       '}',
+                                       'return', 'name', // FIXME
+                                       '===',
+                                       "'input'",
+                                       ';',
+                                       '}',
+                                       ')',
+                                       ';',
+                               ]
+                       ]
                ];
        }
 
        /**
-        * @dataProvider provideExponentLineBreaking
+        * @dataProvider provideLineBreaker
         * @covers JavaScriptMinifier::minify
         */
-       public function testExponentLineBreaking( $num ) {
-               // Long line breaking was being incorrectly done between the base and
-               // exponent part of a number, causing a syntax error. The line should
-               // instead break at the start of the number. (T34548)
-               $prefix = 'var longVarName' . str_repeat( '_', 973 ) . '=';
-               $suffix = ',shortVarName=0;';
-
-               $input = $prefix . $num . $suffix;
-               $expected = $prefix . "\n" . $num . $suffix;
-
-               $minified = JavaScriptMinifier::minify( $input );
-
-               $this->assertEquals( $expected, $minified, "Line breaks must not occur in middle of exponent" );
+       public function testLineBreaker( $code, array $expectedLines ) {
+               $this->setMaxLineLength( 1 );
+               $actual = JavaScriptMinifier::minify( $code );
+               $this->assertEquals(
+                       array_merge( [ '' ], $expectedLines ),
+                       explode( "\n", $actual )
+               );
        }
 }