Page MenuHomePhabricator

Load /tests/parser/ParserTestParserHook.php when testing extensions
Closed, ResolvedPublic

Description

I recently submitted a patch to the LabeledSectionTransclusion extension, and included a few new parser tests for it. These new parser tests make use of the <tag> tag that is defined in the file /tests/parser/ParserTestParserHook.php and is meant for testing and debugging purposes. This file is loaded automatically when you run the parser tests manually, with a command such as this one:

php tests/parser/parserTests.php --file=extensions/LabeledSectionTransclusion/lstParserTests.txt

I wrote my patch and run the tests locally with the above command and the latest MediaWiki (1.30), they all pass. However, when I sent it for review, Jenkins throws an error (see the bottom of https://integration.wikimedia.org/ci/job/mwext-testextension-hhvm-jessie/6366/console) with the following message:

Undefined property: PPNode_Hash_Array::$node

I'm not 100% sure, but I believe this error is due to Jenkins not loading the ParserTestParserHook.php file that defines the <tag> tag.

Event Timeline

I can not reproduce locally :-(

$ php tests/phpunit/phpunit.php --testsuite extensions --filter lstParserTests.txt
Using PHP 5.6.30-0+deb8u1
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

......................................F.

Time: 2.44 seconds, Memory: 62.25MB

There was 1 failure:

1) ParserIntegrationTest::testParse with data set "lstParserTests.txt: LST: Transclude a nested tag" ('[details omitted]')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'<pre>
-'<section begin=content />tag<section end=content />'
-array (
-)
-</pre>
-'
+'<p>&lt;tag&gt;tag
+</p>'

/home/hashar/projects/mediawiki/core/tests/phpunit/includes/parser/ParserIntegrationTest.php:37
/home/hashar/projects/mediawiki/core/maintenance/doMaintenance.php:111

FAILURES!
Tests: 40, Assertions: 40, Failures: 1.

Change 356016 had a related patch set uploaded (by Hashar; owner: Hashar):
[mediawiki/extensions/LabeledSectionTransclusion@master] Reproduction case for T166480

https://gerrit.wikimedia.org/r/356016

I have found the issue! The patch relies on the DOM parser preprocessor when CI somehow uses the Hash preprocessor.

LabeledSectionTransclusion.class.php
391 } else {
392     // If we're in an extension tag (such as <poem> or <ref>), add the 'ext' nodes inside to the queue
393     $dom = $parser->preprocessToDom( $parts['inner'] );
394     foreach ( $dom->getChildrenOfType( 'ext' )->node as $child ) {
395         $child = new PPNode_DOM( $child );
396         $queue[] = $child;
397     }
398 }

One can fetch https://gerrit.wikimedia.org/r/#/c/356016/ to set the preprocessor to hash and would then reproduce the issue CI encounters.

The fix would be to make your code agnostic to the HASH/DOM preprocessor. But I am unfamiliar with the Parser code or what is the intent in this code. I would assume getChildrenOfType() to return an array of PPNode which maybe you could directly append in the $queue?

That piece of code is rather delicate (it took me quite a while to craft) so rather than touching it, I thought I could add the following to the parser tests:

!! options
preprocessor=Preprocessor_DOM

The patch passed again the basic test build, but I run the "recheck" command and Jenkins didn't respond. I guess I don't have the rights to command him. Could you try, please? Thanks!

The patch passed again the basic test build, but I run the "recheck" command and Jenkins didn't respond. I guess I don't have the rights to command him. Could you try, please? Thanks!

I have added you to the CI configuration with https://gerrit.wikimedia.org/r/#/c/356147/ should be good now.

That piece of code is rather delicate (it took me quite a while to craft) so rather than touching it, I thought I could add the following to the parser tests:

!! options
preprocessor=Preprocessor_DOM

Yes that work around the issue which is the implementation is only meant for the DOM Preprocessor. So whenever one uses the Hash one the extension will not work!

Lets follow up on your Gerrit change.

hashar claimed this task.