Upgrade to add intl PHP extension has broken these tests. Reverting for now, but we'll need this fixed. :-(
Description
Details
Status | Subtype | Assigned | Task | ||
---|---|---|---|---|---|
Resolved | Reedy | T227242 Deploy WebAuthn to Wikimedia Wikis | |||
Resolved | Jdforrester-WMF | T236907 mwgate-composer-php72-docker missing php-intl | |||
Resolved | Anomie | T237049 DifferenceEngineTest and ApiComparePagesTest failures when wikidiff2 is installed |
Event Timeline
Change 547642 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[integration/config@master] Revert "jjb: Bump all jobs for new php71/72/73 image cascade"
Change 547642 merged by jenkins-bot:
[integration/config@master] Revert "jjb: Bump all jobs for new php71/72/73 image cascade"
15:34:43 Failed asserting that two arrays are equal. 15:34:43 --- Expected 15:34:43 +++ Actual 15:34:43 @@ @@ 15:34:43 - 'body' => '<tr><td colspan="2" class="diff-lineno" id="mw-diff-left-l1" >Line 1:</td>\n 15:34:43 - <td colspan="2" class="diff-lineno">Line 1:</td></tr>\n 15:34:43 - <tr><td class='diff-marker'>−</td><td class='diff-deletedline'><div><del class="diffchange diffchange-inline">A </del>4</div></td><td class='diff-marker'>+</td><td class='diff-addedline'><div><ins class="diffchange diffchange-inline">B </ins>4</div></td></tr>\n 15:34:43 + 'body' => '<tr>\n 15:34:43 + <td colspan="2" class="diff-lineno">Line 1:</td>\n 15:34:43 + <td colspan="2" class="diff-lineno">Line 1:</td>\n 15:34:43 + </tr>\n 15:34:43 + <tr>\n 15:34:43 + <td class="diff-marker">−</td>\n 15:34:43 + <td class="diff-deletedline"><div><del class="diffchange diffchange-inline">A</del> 4</div></td>\n 15:34:43 + <td class="diff-marker">+</td>\n 15:34:43 + <td class="diff-addedline"><div><ins class="diffchange diffchange-inline">B</ins> 4</div></td>\n 15:34:43 + </tr>\n
The "expected" looks like what TableDiffFormatter.php would produce (e.g. header from here), while the "actual" looks like wikidiff2 instead (e.g. header from here).
Presumably $this->engine is evaluating to ENGINE_PHP (default) instead of ENGINE_WIKIDIFF2.
setEngine() is being called from getSlotDiffRendererInternal. wgExternalDiffEngine defaults to false. Is wikidiff2_do_diff somehow not showing up any more?
I think you have it backwards. The tests are expecting to be run without wikidiff2, but somehow the CI tests have started using it.
@Jdforrester-WMF Yeah, that was for our team to support integration tests against the MW REST API endpoints that use Wikidiff2, did we cause knock-on breakages?
Yes. We can't update CI for any of MW core/extensions/skins right now. Will write a patch moving these tests in MW to a lower level.
Actually, no. I've spent three hours trying to disentangle this mess and I'm deep in the bowels of ContentHandler and its assumptions.
@daniel is probably needed to fix this, sorry.
I'm traveling/conferencing the next couple of weeks, so won't happen soon. Maybe @Tgr can give some hints.
$wgExternalDiffEngine defaults to false which also mean it would first use wikidiff2 when the function wikidiff2_do_diff is detected. There is no way to force the php native one :-\
Abstract:
public static function getEngine() { $externalDiffEngine = MediaWikiServices::getInstance()->getMainConfig() ->get( 'ExternalDiffEngine' ); ... if ( function_exists( 'wikidiff2_do_diff' ) ) { return 'wikidiff2'; } // Native PHP return false; }
So that always return wikidiff2.
I guess we want to enhance the setting ExternalDiffEngine to allow php native. This way the couple tests failing can be forced to use the native php rendering.
The task description refers to the addition of the intl extension. That does not seem related. I guess it is because the CI image had wikidiff2 added but only the api-testing job had been switched to it?
Yup, pretty much. Adding intl (and James pushing it out) was the action that pushed also the wikidiff2 changes out to numerous other CI images. Which should've been done originally
Given other pressing needs, I'd recommend reverting the addition of wikidiff2 to the quibble images (which will mean either the API testing suite stops working, or we hold it back at the moderately-broken stage).
The traditional hack has been to set the diff engine to something like /dev/null, but rMW5b3bbd5adea3: Drop strings for wgExternalDiffEngine, deprecated in 1.27 and 1.32 broke that; IIUC the breakage did not manifest on that patch because the CI environment did not have wikidiff installed at the time.
So the simplest short-term fix would be just reverting that. Or maybe replacing with something less hacky where there is a special value you can pass (native or whatever) for using the PHP engine.
Change 548763 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] Add $wgDiffEngine
All tests that rely on a specific diffengine implementation should set it themselves (and from glancing at Brad's patch, they already do).
Ok, cool. So its just attempting to roll out the newer CI docker images everywhere then :)
That fix it for master but the release branches would face the same issue. The CI containers are the same for all branches and thus all include wikidiff2.
We might have to backport $wgDiffEngine to our previous releases (REL1_31, REL1_32 and REL1_33).
(that is until we figure out how to have Ci to come without any extensions loaded and load them selectively based on what the repositories require. But that is a different story).
Change 549086 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[integration/config@master] Re-apply "jjb: Bump all jobs for new php71/72/73 image cascade"
Change 549088 had a related patch set uploaded (by Jforrester; owner: Anomie):
[mediawiki/core@REL1_34] Add $wgDiffEngine
Mentioned in SAL (#wikimedia-releng) [2019-11-06T15:40:31Z] <James_F> Upgraded mediawiki-quibble-composer-mysql-php72-docker as a test flight for T236907 T237049
Change 549086 merged by jenkins-bot:
[integration/config@master] Re-apply "jjb: Bump all jobs for new php71/72/73 image cascade"
I've backported to REL1_34; 1.31/1.32/1.33 are unaffected, so all passed already.
Huge thanks to @Anomie, as ever.
Change 549670 had a related patch set uploaded (by Krinkle; owner: Anomie):
[mediawiki/core@wmf/1.35.0-wmf.5] Add $wgDiffEngine
Change 549670 merged by jenkins-bot:
[mediawiki/core@wmf/1.35.0-wmf.5] Add $wgDiffEngine