From 361283a332342d653ee990de7809f4fbe4e6401d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartosz=20Dziewo=C5=84ski?= Date: Wed, 12 Oct 2022 22:03:23 +0200 Subject: [PATCH] Ship HTML test files for JS using 'packageFiles' instead of 'templates' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We originally used 'templates' because it seemed like an obvious choice for HTML files, and because 'packageFiles' requires extra code to include anything that isn't a .js or .json file. However, the templates are expected to be HTML fragments rather than whole documents, and they are parsed in a particular way that takes a lot of code to clean up (which we needed to do, because we use the same test files for testing PHP code). I tried doing it in the 'packageFiles' way, and the extra code doesn't seem that bad in comparison after all. Moreover, the 'templates' mechanism (when used the intended way) feels vaguely deprecated in favor of Vue.js, and I'd rather move away from it. This makes the tests faster too (probably mostly thanks to the removal of the clean up code) – on my machine they go from 1800ms to 1500ms. (Simplify linearWalk tests, as we no longer need to do weird things with document fragments to get consistent outputs in PHP and JS.) Change-Id: I39f9b994ce5636d70fea2e935a7c87c7d56dcb26 --- includes/ResourceLoaderData.php | 12 +++++++- tests/cases/linearWalk/simple.json | 12 ++++++-- tests/phpunit/unit/CommentUtilsTest.php | 9 ++---- tests/qunit/modifier.test.js | 40 +++++++++++-------------- tests/qunit/parser.test.js | 9 +++--- tests/qunit/testUtils.js | 19 +++++------- tests/qunit/utils.test.js | 6 ++-- 7 files changed, 55 insertions(+), 52 deletions(-) diff --git a/includes/ResourceLoaderData.php b/includes/ResourceLoaderData.php index 7a90c1237..b4290fd9f 100644 --- a/includes/ResourceLoaderData.php +++ b/includes/ResourceLoaderData.php @@ -169,7 +169,17 @@ class ResourceLoaderData { if ( str_ends_with( $val, '.json' ) ) { $info['packageFiles'][] = substr( $val, strlen( '../' ) ); } elseif ( str_ends_with( $val, '.html' ) ) { - $info['templates'][] = $val; + $info['packageFiles'][] = [ + 'name' => $val, + 'type' => 'data', + 'callback' => static function () use ( $info, $val ) { + $localPath = $info['localBasePath'] . '/' . $val; + return file_get_contents( $localPath ); + }, + 'versionCallback' => static function () use ( $val ) { + return new RL\FilePath( $val ); + }, + ]; } } } diff --git a/tests/cases/linearWalk/simple.json b/tests/cases/linearWalk/simple.json index 665324127..40aff534a 100644 --- a/tests/cases/linearWalk/simple.json +++ b/tests/cases/linearWalk/simple.json @@ -1,5 +1,9 @@ [ - "enter #document-fragment(11)", + "enter #document(9)", + "enter html(1)", + "enter head(1)", + "leave head(1)", + "enter body(1)", "enter p(1)", "enter b(1)", "enter a(1)", @@ -146,5 +150,9 @@ "enter #text(3)", "leave #text(3)", "leave p(1)", - "leave #document-fragment(11)" + "enter #text(3)", + "leave #text(3)", + "leave body(1)", + "leave html(1)", + "leave #document(9)" ] diff --git a/tests/phpunit/unit/CommentUtilsTest.php b/tests/phpunit/unit/CommentUtilsTest.php index 905819757..cf05b3f1d 100644 --- a/tests/phpunit/unit/CommentUtilsTest.php +++ b/tests/phpunit/unit/CommentUtilsTest.php @@ -5,7 +5,6 @@ namespace MediaWiki\Extension\DiscussionTools\Tests\Unit; use MediaWiki\Extension\DiscussionTools\CommentUtils; use MediaWiki\Extension\DiscussionTools\Tests\TestUtils; use MediaWikiUnitTestCase; -use Wikimedia\Parsoid\Utils\DOMCompat; /** * @group DiscussionTools @@ -20,18 +19,16 @@ class CommentUtilsTest extends MediaWikiUnitTestCase { */ public function testLinearWalk( string $name, string $htmlPath, string $expectedPath ) { $html = static::getHtml( $htmlPath ); - // Slightly awkward to get the same output as in the JS version - $fragment = ( DOMCompat::newDocument( true ) )->createDocumentFragment(); - $fragment->appendXML( trim( $html ) ); + $doc = static::createDocument( $html ); $expected = static::getJson( $expectedPath ); $actual = []; - CommentUtils::linearWalk( $fragment, static function ( $event, $node ) use ( &$actual ) { + CommentUtils::linearWalk( $doc, static function ( $event, $node ) use ( &$actual ) { $actual[] = "$event {$node->nodeName}({$node->nodeType})"; } ); $actualBackwards = []; - CommentUtils::linearWalkBackwards( $fragment, static function ( $event, $node ) use ( &$actualBackwards ) { + CommentUtils::linearWalkBackwards( $doc, static function ( $event, $node ) use ( &$actualBackwards ) { $actualBackwards[] = "$event {$node->nodeName}({$node->nodeType})"; } ); diff --git a/tests/qunit/modifier.test.js b/tests/qunit/modifier.test.js index 257a96141..d520fdad8 100644 --- a/tests/qunit/modifier.test.js +++ b/tests/qunit/modifier.test.js @@ -21,23 +21,19 @@ require( '../cases/modified.json' ).forEach( function ( caseItem ) { return; } QUnit.test( testName, function ( assert ) { - var fixture = document.getElementById( 'qunit-fixture' ); - - var dom = mw.template.get( 'test.DiscussionTools', caseItem.dom ).render(), - expected = mw.template.get( 'test.DiscussionTools', caseItem.expected ).render(), + var dom = ve.createDocumentFromHtml( require( '../' + caseItem.dom ) ), + expected = ve.createDocumentFromHtml( require( '../' + caseItem.expected ) ), config = require( caseItem.config ), data = require( caseItem.data ), title = mw.Title.newFromText( caseItem.title ); testUtils.overrideMwConfig( config ); - $( fixture ).empty().append( testUtils.getThreadContainer( expected ).children() ); - var expectedHtml = fixture.innerHTML; + var expectedHtml = testUtils.getThreadContainer( expected ).innerHTML; + var reverseExpectedHtml = testUtils.getThreadContainer( dom ).innerHTML; - $( fixture ).empty().append( testUtils.getThreadContainer( dom ).children() ); - var reverseExpectedHtml = fixture.innerHTML; - - var threadItemSet = new Parser( data ).parse( fixture, title ); + var container = testUtils.getThreadContainer( dom ); + var threadItemSet = new Parser( data ).parse( container, title ); var comments = threadItemSet.getCommentItems(); // Add a reply to every comment. Note that this inserts *all* of the replies, unlike the real @@ -51,9 +47,9 @@ require( '../cases/modified.json' ).forEach( function ( caseItem ) { } ); // Uncomment this to get updated content for the "modified HTML" files, for copy/paste: - // console.log( fixture.innerHTML ); + // console.log( container.innerHTML ); - var actualHtml = fixture.innerHTML; + var actualHtml = container.innerHTML; assert.strictEqual( actualHtml, @@ -66,7 +62,7 @@ require( '../cases/modified.json' ).forEach( function ( caseItem ) { modifier.removeAddedListItem( node ); } ); - var reverseActualHtml = fixture.innerHTML; + var reverseActualHtml = container.innerHTML; assert.strictEqual( reverseActualHtml, reverseExpectedHtml, @@ -76,24 +72,22 @@ require( '../cases/modified.json' ).forEach( function ( caseItem ) { } ); QUnit.test( '#addReplyLink', function ( assert ) { - var cases = require( '../cases/reply.json' ), - fixture = document.getElementById( 'qunit-fixture' ); + var cases = require( '../cases/reply.json' ); cases.forEach( function ( caseItem ) { - var dom = mw.template.get( 'test.DiscussionTools', caseItem.dom ).render(), - expected = mw.template.get( 'test.DiscussionTools', caseItem.expected ).render(), + var dom = ve.createDocumentFromHtml( require( '../' + caseItem.dom ) ), + expected = ve.createDocumentFromHtml( require( '../' + caseItem.expected ) ), config = require( caseItem.config ), data = require( caseItem.data ), title = mw.Title.newFromText( caseItem.title ); testUtils.overrideMwConfig( config ); - $( fixture ).empty().append( testUtils.getThreadContainer( expected ).children() ); - var expectedHtml = fixture.innerHTML; + var expectedHtml = testUtils.getThreadContainer( expected ).innerHTML; - $( fixture ).empty().append( testUtils.getThreadContainer( dom ).children() ); + var container = testUtils.getThreadContainer( dom ); - var threadItemSet = new Parser( data ).parse( fixture, title ); + var threadItemSet = new Parser( data ).parse( container, title ); var comments = threadItemSet.getCommentItems(); // Add a reply link to every comment. @@ -105,9 +99,9 @@ QUnit.test( '#addReplyLink', function ( assert ) { } ); // Uncomment this to get updated content for the "reply HTML" files, for copy/paste: - // console.log( fixture.innerHTML ); + // console.log( container.innerHTML ); - var actualHtml = fixture.innerHTML; + var actualHtml = container.innerHTML; assert.strictEqual( actualHtml, diff --git a/tests/qunit/parser.test.js b/tests/qunit/parser.test.js index 6bded2bcb..6e418a8e7 100644 --- a/tests/qunit/parser.test.js +++ b/tests/qunit/parser.test.js @@ -85,21 +85,20 @@ require( '../cases/comments.json' ).forEach( function ( caseItem ) { } QUnit.test( testName, function ( assert ) { - var fixture = document.getElementById( 'qunit-fixture' ); - var $dom = mw.template.get( 'test.DiscussionTools', caseItem.dom ).render(), + var dom = ve.createDocumentFromHtml( require( '../' + caseItem.dom ) ), expected = require( caseItem.expected ), config = require( caseItem.config ), data = require( caseItem.data ), title = mw.Title.newFromText( caseItem.title ); - $( fixture ).empty().append( testUtils.getThreadContainer( $dom ).children() ); + var container = testUtils.getThreadContainer( dom ); testUtils.overrideMwConfig( config ); - var threadItemSet = new Parser( data ).parse( fixture, title ); + var threadItemSet = new Parser( data ).parse( container, title ); var threads = threadItemSet.getThreads(); threads.forEach( function ( thread, i ) { - testUtils.serializeComments( thread, fixture ); + testUtils.serializeComments( thread, container ); assert.deepEqual( JSON.parse( JSON.stringify( thread ) ), diff --git a/tests/qunit/testUtils.js b/tests/qunit/testUtils.js index 8f87ff08d..66772fd22 100644 --- a/tests/qunit/testUtils.js +++ b/tests/qunit/testUtils.js @@ -18,20 +18,15 @@ module.exports.overrideMwConfig = function ( config ) { /** * Return the node that is expected to contain thread items. * - * @param {jQuery} $nodes - * @return {jQuery} + * @param {Document} doc + * @return {Element} */ -module.exports.getThreadContainer = function ( $nodes ) { - // In tests created from Parsoid output, comments are contained directly in . This becomes a - // huge mess of
nodes when we insert it into the existing document, oh well… +module.exports.getThreadContainer = function ( doc ) { + // In tests created from Parsoid output, comments are contained directly in . // In tests created from old parser output, comments are contained in
. - if ( $nodes.filter( 'section' ).length ) { - return $( '
' ) - .append( $nodes.filter( 'section' ) ) - .append( $nodes.filter( 'base' ) ); - } else { - return $nodes.find( 'div.mw-parser-output' ).addBack( 'div.mw-parser-output' ); - } + var body = doc.body; + var wrapper = body.querySelector( 'div.mw-parser-output' ); + return wrapper || body; }; /** diff --git a/tests/qunit/utils.test.js b/tests/qunit/utils.test.js index 1508bd12e..1b7ff1466 100644 --- a/tests/qunit/utils.test.js +++ b/tests/qunit/utils.test.js @@ -7,16 +7,16 @@ QUnit.test( '#linearWalk', function ( assert ) { cases.forEach( function ( caseItem ) { var - $dom = mw.template.get( 'test.DiscussionTools', caseItem.dom ).render(), + doc = ve.createDocumentFromHtml( require( '../' + caseItem.dom ) ), expected = require( caseItem.expected ); var actual = []; - utils.linearWalk( $dom[ 0 ].parentNode, function ( event, node ) { + utils.linearWalk( doc, function ( event, node ) { actual.push( event + ' ' + node.nodeName.toLowerCase() + '(' + node.nodeType + ')' ); } ); var actualBackwards = []; - utils.linearWalkBackwards( $dom[ 0 ].parentNode, function ( event, node ) { + utils.linearWalkBackwards( doc, function ( event, node ) { actualBackwards.push( event + ' ' + node.nodeName.toLowerCase() + '(' + node.nodeType + ')' ); } );