Page MenuHomePhabricator

DifferenceEngineTest and ApiComparePagesTest failures when wikidiff2 is installed
Closed, ResolvedPublic

Description

Upgrade to add intl PHP extension has broken these tests. Reverting for now, but we'll need this fixed. :-(

Details

Related Gerrit Patches:
mediawiki/core : wmf/1.35.0-wmf.5Add $wgDiffEngine
mediawiki/core : masterAdd $wgDiffEngine
integration/config : masterRe-apply "jjb: Bump all jobs for new php71/72/73 image cascade"
mediawiki/core : REL1_34Add $wgDiffEngine
integration/config : masterRevert "jjb: Bump all jobs for new php71/72/73 image cascade"

Event Timeline

Jdforrester-WMF triaged this task as High priority.Thu, Oct 31, 7:47 PM
Jdforrester-WMF created this task.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptThu, Oct 31, 7:47 PM

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"

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

Change 547642 merged by jenkins-bot:
[integration/config@master] Revert "jjb: Bump all jobs for new php71/72/73 image cascade"

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

Anomie added a subscriber: Anomie.Thu, Oct 31, 8:16 PM
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?

Anomie added a comment.Fri, Nov 1, 1:43 PM

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.

Aha, right, well, that makes sense because of 34c8b6e0 for T236333.

*sighs*

Jdforrester-WMF renamed this task from DifferenceEngineTest and ApiComparePagesTest failures when intl is installed to DifferenceEngineTest and ApiComparePagesTest failures when wikidiff2 is installed.Fri, Nov 1, 5:12 PM
Jdforrester-WMF added a project: wikidiff2.

@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?

@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.

Jdforrester-WMF added a subscriber: daniel.EditedMon, Nov 4, 6:26 PM

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.

daniel added a subscriber: Tgr.

@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.

hashar added a comment.Mon, Nov 4, 8:53 PM

$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:

includes/diff/DifferenceEngine.php
    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?

Reedy added a subscriber: Reedy.Mon, Nov 4, 9:04 PM

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).

Tgr added a comment.Mon, Nov 4, 9:17 PM

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

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

Change 548763 merged by jenkins-bot:
[mediawiki/core@master] Add $wgDiffEngine

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

Reedy added a comment.Tue, Nov 5, 11:38 PM

What's next? Set $wgDiffEngine in CI to PHP, and roll out the updated images again?

Tgr added a comment.Tue, Nov 5, 11:42 PM

All tests that rely on a specific diffengine implementation should set it themselves (and from glancing at Brad's patch, they already do).

Reedy closed this task as Resolved.Wed, Nov 6, 12:20 AM
Reedy assigned this task to Anomie.

Ok, cool. So its just attempting to roll out the newer CI docker images everywhere then :)

hashar reopened this task as Open.Wed, Nov 6, 9:45 AM

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"

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

Change 549088 had a related patch set uploaded (by Jforrester; owner: Anomie):
[mediawiki/core@REL1_34] Add $wgDiffEngine

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

Change 549088 merged by jenkins-bot:
[mediawiki/core@REL1_34] Add $wgDiffEngine

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

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"

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

Jdforrester-WMF closed this task as Resolved.Wed, Nov 6, 4:07 PM

We might have to backport $wgDiffEngine to our previous releases (REL1_31, REL1_32 and REL1_33).

I've backported to REL1_34; 1.31/1.32/1.33 are unaffected, so all passed already.

Huge thanks to @Anomie, as ever.

hashar added a comment.Thu, Nov 7, 1:56 PM

Awesome, thank you for the backport to REL1_34!

Change 549670 had a related patch set uploaded (by Krinkle; owner: Anomie):
[mediawiki/core@wmf/1.35.0-wmf.5] Add $wgDiffEngine

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

Change 549670 merged by jenkins-bot:
[mediawiki/core@wmf/1.35.0-wmf.5] Add $wgDiffEngine

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