From: Timo Tijhof <krinklemail@gmail.com>
Date: Fri, 10 Aug 2018 22:11:53 +0000 (+0100)
Subject: JavaScriptMinifier: Add better line-breaker tests
X-Git-Tag: 1.34.0-rc.0~4488^2
X-Git-Url: http://git.cyclocoop.org/?a=commitdiff_plain;h=8cfe4abf8d707b68141a281c8f8c8d2c0f5190c9;p=lhc%2Fweb%2Fwiklou.git

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 aaf62b5b11..8541c4f7ed 100644
--- 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 03a4438641..70f9c7ca42 100644
--- 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 )
+		);
 	}
 }