Jenkins is broken on master
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Ladsgroup
	Jan 18 2017, 1:37 AM

Description

First issue was fixed:

https://gerrit.wikimedia.org/r/#/c/332701/

I'm looking into this. Hopefully I can fix it.

Second is Scribunto tests failing due to a title having an interwiki which is not properly interpolated.

OK	Jan 17th 17:06 UTC	https://gerrit.wikimedia.org/r/#/c/332494/
FAILED	Jan 17th 21:28 UTC	https://gerrit.wikimedia.org/r/#/c/332442/3

Details

Subject	Repo	Branch	Lines +/-
test: reset InterWikiLookup service after test	mediawiki/extensions/CirrusSearch	master	+6 -0
(DO NOT SUBMIT) test CirrusSearch fixup	mediawiki/extensions/Wikibase	master	+0 -0
test: change interwiki to a meaningful name	mediawiki/extensions/Scribunto	master	+8 -8
Do not try to update user counts for anonymous users	mediawiki/extensions/BetaFeatures	master	+7 -4
Do not run getOption when user is not valid	mediawiki/extensions/BetaFeatures	master	+3 -0
Use a TestUser in EntityContentFactoryTest::testGetPermissionStatusForEntity	mediawiki/extensions/Wikibase	master	+30 -12
Don't use $wgUser in EntityContentFactoryTest::testGetPermissionStatusForEntity	mediawiki/extensions/Wikibase	master	+28 -17

Customize query in gerrit

Related Objects

Mentioned Here: rECIR647c03095a2d: Add and implement InterwikiResolver
rCICF0874d170309e: Update CirrusSearch extension dependencies

Event Timeline

Ladsgroup created this task.Jan 18 2017, 1:37 AM

Restricted Application added subscribers: Jay8g, TerraCodes, Aklapper. · View Herald TranscriptJan 18 2017, 1:37 AM

Change 332718 had a related patch set uploaded (by Ladsgroup):
Don't use $wgUser in EntityContentFactoryTest::testGetPermissionStatusForEntity

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

gerritbot added a project: Patch-For-Review.Jan 18 2017, 2:24 AM

Here's my hypotheses:
Given that all of the code base related to the test were untouched. It seems jenkins somehow makes a user with empty string as username as inject it as $wgUser and when we are changing user's permission, the beta feature extension tries to re-build the user from username and it errors out. This patch should fix it.

Change 332718 abandoned by Ladsgroup:
Don't use $wgUser in EntityContentFactoryTest::testGetPermissionStatusForEntity

Reason:
I ran out of ideas. it looks more complex. I might follow other ways to get this working

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

Change 332727 had a related patch set uploaded (by Ladsgroup):
Do not run getOption when user is not valid

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

Peachey88 merged a task: T155599: Jenkins is broken on master.Jan 18 2017, 7:42 AM

Here's my hypotheses:
Given that all of the code base related to the test were untouched. It seems jenkins somehow makes a user with empty string as username as inject it as $wgUser and when we are changing user's permission, the beta feature extension tries to re-build the user from username and it errors out. This patch should fix it.

Something along these lines, possibly.

I managed to reproduce the problem locally and having looked into it quite briefly it seems what Wikibase tests do has not much to do with the problem itself. Those tests just stash $wgUser using Mediawiki's general mechanism, so it should not be the source of the problem IMO. Actually, I managed to have tests break even the user was an IP before and after stashing.
I'd guess tests started to fail on Jenkins because some extension in the CI setup has been enabled as beta feature recently, and before there were no beta feature on the test setup and the relevant code in BetaFeaturesHooks::updateUserCounts hasn't even been executed. Therefore I think the solution you proposed in https://gerrit.wikimedia.org/r/332727 is the right thing to do.
That said I didn't manage to find out was there actually any change to the CI setup recently that would have an influence on this.

I came to the same conclusion as @WMDE-leszek. It seems a random extension (not Wikibase and not BetaFeatures, but an other on on the CI) started exposing a beta feature. All extensions doing this are hooking into the GetBetaFeaturePreferences hook, and are adding an entry to the $betaFeatures array. The moment this array is not empty, the code in the loop is executed, and starts failing for anonymous users.

Change 332750 had a related patch set uploaded (by Thiemo Mättig (WMDE)):
Do not try to update user counts for anonymous users

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

Change 332750 merged by jenkins-bot:
Do not try to update user counts for anonymous users

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

Change 332727 abandoned by Ladsgroup:
Do not run getOption when user is not valid

Reason:
Done in I2a09062

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

ReleaseTaggerBot added a project: MW-1.29-release (WMF-deploy-2017-01-24_(1.29.0-wmf.9)).Jan 18 2017, 3:00 PM

So is this now fixed?

The original reason why we opened this ticket is resolved. Now Scribunto fails for an other reason.

[09:00:16]  <Amir1>	hashar: Hey, Jenkins is broken on master of Wikibase (https://phabricator.wikimedia.org/T155600) due to failed Scribunto tests, but Scribunto extensions hasn't been touched for a while. I was wondering if some config changes can cause this
[09:00:31]  <Amir1>	An example of the failure: https://integration.wikimedia.org/ci/job/mwext-testextension-php55-composer/7550/console
[09:01:07]  <Amir1>	Did you change anything related to lua/scribunto recently?
[09:07:07]  <+hashar>	Might want to link to https://integration.wikimedia.org/ci/job/mwext-testextension-php55-composer/7550/testReport/
[09:07:19]  <+hashar>	Amir1: from a quick look, I guess we have changed somethings Wikibase or the CI Config
[09:07:40]  <+hashar>	and the wiki sites table in MediaWiki or Wikibase changed
[09:08:23]  <+hashar>	so something changed somewhere and most probably fairly recently
[09:08:34]  <+hashar>	the last change that worked fine was on Jan 12 th https://gerrit.wikimedia.org/r/#/c/331909/

Example of a test failure:

LuaSandbox: TitleLibraryTests[47]: .fullUrl()
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
 {
   "//wiki.local/wiki/Main_Page",
   "//wiki.local/wiki/Module:TestFramework",
-  "//test.wikipedia.org/wiki/Module:TestFramework",
+  "//wiki.local/wiki/scribuntotitletest:Module:TestFramework",
   "//wiki.local/wiki/Talk:Has/A/Subpage#frag",
   "//wiki.local/wiki/Not/A/Subpage",
   "//wiki.local/wiki/Module_talk:Test_Framework#_frag_frag",
   "//wiki.local/wiki/Module_talk:Test_Framework#_frag_frag",
 }

The last change that worked fine for Scribunto was on Jan 12 th https://gerrit.wikimedia.org/r/#/c/331909/

Tried a dummy Scribunto change https://gerrit.wikimedia.org/r/#/c/43176/ and tests pass just fine https://integration.wikimedia.org/ci/job/mwext-testextension-php55/33212/console So at least standalone Scribunto is all fine.

It might be an issue that leaked in Wikibase, though the job should have failed and prevent it from merging. The jobs triggering on Wikibase have extension being injected:

mediawiki/extensions/BetaFeatures
mediawiki/extensions/Capiunto
mediawiki/extensions/CirrusSearch
mediawiki/extensions/Cite
mediawiki/extensions/Elastica
mediawiki/extensions/GeoData
mediawiki/extensions/MwEmbedSupport
mediawiki/extensions/PdfHandler
mediawiki/extensions/Scribunto
mediawiki/extensions/SiteMatrix
mediawiki/extensions/TimedMediaHandler
mediawiki/extensions/VisualEditor
mediawiki/extensions/cldr

In Scribunto, the testsuite inject an interwiki with prefix scribuntotitletest with the interwiki URL //test.wikipedia.org/wiki/$1.

The test suite then invokes various getUrl method for scribuntotitletest:Module:TestFramework and somehow the interwiki is not recognized :(

Change 332966 had a related patch set uploaded (by Hashar):
test: change interwiki to a meaningful name

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

Paladox subscribed.Jan 19 2017, 8:52 AM

Looking at Wikibase changes, I am trying to narrow down a time window for the failure/regression:

OK	Jan 17th 17:06 UTC	https://gerrit.wikimedia.org/r/#/c/332494/
FAILED	Jan 17th 21:28 UTC	https://gerrit.wikimedia.org/r/#/c/332442/3

Change 332966 merged by jenkins-bot:
test: change interwiki to a meaningful name

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

A suspect is mediawiki/core change OutputPage: Ignore protocol-relative urls in transformResourcePath() https://gerrit.wikimedia.org/r/332730

I have sent a revert patch https://gerrit.wikimedia.org/r/#/c/332972/ . Made a Wikibase change to depends on it https://gerrit.wikimedia.org/r/#/c/70395/ . Still fails so I guess that rules out that specific mediawiki/core change (which is really just for ResourceLoader).

All builds of mwext-testextension-hhvm-composer that have a unit XML file matching //wiki.local/wiki/scribuntotitletest:Module:TestFramework.

The first failure is on https://gerrit.wikimedia.org/r/#/c/332442/3 on Jan 17th 21:28 UTC

hashar updated the task description. (Show Details)Jan 19 2017, 9:33 AM

Mentioned in SAL (#wikimedia-releng) [2017-01-19T09:36:35Z] <hashar> Nuking workspaces of all mwext-testextension-hhvm-composer* jobs. Lame attempt for T155600. salt -v '*slave*' cmd.run 'rm -fR /srv/jenkins-workspace/workspace/mwext-testextension-hhvm-composer*'

In T155600#2951890, @hashar wrote:

In Scribunto, the testsuite inject an interwiki with prefix scribuntotitletest with the interwiki URL //test.wikipedia.org/wiki/$1.

The test suite then invokes various getUrl method for scribuntotitletest:Module:TestFramework and somehow the interwiki is not recognized :(

FWIW it looks like only URL-related tests have some problems with this interwiki. Tests for other parts of Title (text, interwiki, subpage etc) pass, and as far as I get it they should be failing if the interwiki was not recognized at all. So maybe there is some problem with generating the URL. But why and where - I have no idea.

For patches triggered by Wikibase:

Last good https://integration.wikimedia.org/ci/job/mwext-testextension-hhvm-composer/9573/
First fail https://integration.wikimedia.org/ci/job/mwext-testextension-hhvm-composer/9579/consoleText

I did a diff of each console and immediately spotted a change in the EXT_DEPENDENCIES parameters:

$ colordiff -U99 deps_ok deps_fail
--- deps_ok	2017-01-19 12:15:02.000000000 +0100
+++ deps_fail	2017-01-19 12:14:50.000000000 +0100
@@ -1,11 +1,13 @@
+mediawiki/extensions/BetaFeatures
 mediawiki/extensions/Capiunto
 mediawiki/extensions/CirrusSearch
 mediawiki/extensions/Cite
 mediawiki/extensions/cldr
 mediawiki/extensions/Elastica
 mediawiki/extensions/GeoData
 mediawiki/extensions/MwEmbedSupport
 mediawiki/extensions/PdfHandler
 mediawiki/extensions/Scribunto
+mediawiki/extensions/SiteMatrix
 mediawiki/extensions/TimedMediaHandler
 mediawiki/extensions/VisualEditor

BetaFeatures / SiteMatrix have been added as a dependency by 0874d170309e66863571265925b818bc1579dad6

$ TZ=C git show --date=local 0874d1703
commit 0874d170309e66863571265925b818bc1579dad6
Author: Erik Bernhardson <ebernhardson@wikimedia.org>
Date:   Tue Jan 17 18:45:07 2017

    Update CirrusSearch extension dependencies
    
    For phan to process correctly a few more extensions that are optionally
    referenced need to be available.
    
    Bug: T153040
    Change-Id: Ic356791c673ebf1315eafe23d682cefa6dfb0625

diff --git a/zuul/parameter_functions.py b/zuul/parameter_functions.py
index 5a5733f2..8cccdb19 100644
--- a/zuul/parameter_functions.py
+++ b/zuul/parameter_functions.py
@@ -129,7 +129,8 @@ def set_parameters(item, job, params):
     'Cite': ['VisualEditor'],
     'Citoid': ['Cite', 'VisualEditor'],
     'CirrusSearch': ['MwEmbedSupport', 'TimedMediaHandler', 'PdfHandler',
-                     'Cite'],
+                     'Cite', 'Elastica', 'GeoData', 'BetaFeatures',
+                     'SiteMatrix'],
     'CodeEditor': ['WikiEditor'],
     'CollaborationKit': ['EventLogging', 'VisualEditor'],
     'ContentTranslation': ['Echo', 'EventLogging', 'GuidedTour',

So I guess SiteMatrix and Scribunto does not play well :(

Gave it a try with all the dependencies loaded but without Wikibase and I can reproduce locally:

LocalSettings.php

wfLoadExtensions( [
        'BetaFeatures',
        'Capiunto',
        'Cite',
        'cldr',
        'Elastica',
        'GeoData',
        'MwEmbedSupport',
        'PdfHandler',
        'SiteMatrix',
        'VisualEditor',
] );

$_legacy_point = [
        'CirrusSearch',
        'Scribunto',
        'TimedMediaHandler',
];
foreach ( $_legacy_point as $_entry_point ) {
        require_once $wgExtensionDirectory . "/$_entry_point/$_entry_point.php";
}

Running solely Scribunto tests pass: php tests/phpunit/phpunit.php ../extensions/Scribunto/tests

With the whole suite (php phpunit.php --testsuite extensions --stop-on-error) that fails though.

Addshore awarded a token.Jan 19 2017, 2:27 PM

Addshore subscribed.

Next step, I deleted every tests directories but the one from Scribunto and CirrusSearch and I reproduce. Then went with a git bisect on CirrusSearch. End result is CirrusSearch commit 647c03095a2ded5b07283186b5f0d1785d603760 https://gerrit.wikimedia.org/r/#/c/320757/ by @dcausse

In short, the CirrusSearch new InterwikiResolver seems to fail to strip some interwiki when extracting a title with Scribunto. I cant tell whether it is a fault in the new resolver or Scribunto that needs some tweak / not using the proper interfaces.

The commit has been done back in November 2016 that is rather too late to revert it :(

Restricted Application added projects: Discovery-ARCHIVED, Discovery-Search. · View Herald TranscriptJan 19 2017, 2:53 PM

Tests no more fails if I drop CirrusSearch tests/unit/InterwikiResolverTest.php.

The class has an help getSiteMatrixInterwikiResolver() which set globals though they are supposedly tear down by MediaWikiTestCase.

Then there is a MediaWiki service InterwikiLookup which is set, but apparently not teardown:

// We need to reset this service so it can load wgInterwikiCache
 MediaWikiServices::getInstance()
     ->resetServiceForTesting( 'InterwikiLookup' );
 $config = new HashSearchConfig( $myGlobals, ['inherit'] );
 $resolver = MediaWikiServices::getInstance()
     ->getService( InterwikiResolverFactory::SERVICE )
     ->getResolver( $config );
 $this->assertEquals( SiteMatrixInterwikiResolver::class, get_class( $resolver ) );
 return $resolver;

Change 333003 had a related patch set uploaded (by Hashar):
test: reset InterWikiLookup service after test

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

Change 333004 had a related patch set uploaded (by Hashar):
(DO NOT SUBMIT) test CirrusSearch fixup

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

Change 333004 abandoned by Hashar:
(DO NOT SUBMIT) test CirrusSearch fixup

Reason:
Confirms that CirrusSearch https://gerrit.wikimedia.org/r/#/c/333003/ is the fix to T155600.

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

Change 333003 merged by jenkins-bot:
test: reset InterWikiLookup service after test

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

I have read the MediaWikiTestCase method. It seems the test should use either one of setService or overrideMwServices() which would tear down the service on completion :]

Looking at InterwikiResolverTest it seems it actually does not mess with services directly. Problem is that core's InterwikiLookups rely on wgInterwikiCache global which InterwikiResolverTest modifies in order to get custom lookup. To make core lookups "re-set" to their standard behaviour after test run not only value of the global variable must be restored (MediaWikiTestCase does it) but also service must be reset to pick up the new value of the global (that bit was missing from test's tearDown).

So InterwikiResolverTest was causing issues as it was not cleaning good enough but the root of all evil is global state in core I think :)

Any way, thank you very much @hashar for investigating this and identifyint the problem.

I did a "recheck" on all Wikibase changes that received a CR+2 but had tests failling.

Lowering priority since the breakage is fixed. A follow up action would be to investigate the use of MediaWikiTestCase::setService() in InterwikiResolverTest. Maybe that can be made a new task.

Else we can just mark this fixed!

Amazing! Thanks you so much, @hashar. As far as I can see this is fixed now.

@WMDE-leszek Danke for the explanation ! That makes sense now :-]

I am fine with closing this task.

EBernhardson closed this task as Resolved.Jan 19 2017, 11:05 PM

Jenkins is broken on masterClosed, ResolvedPublicActions

Description

Details

Related Objects

Event Timeline

Jenkins is broken on master
Closed, ResolvedPublic
Actions