Page MenuHomePhabricator

mwext-testextension-* jobs does not remove skins from previous runs
Closed, ResolvedPublic

Description

T113839 is a failure of mwext-testextension-zend job on the Disambiguator extension about some issue with Metrolook. skins/Metrolook is not a dependency, but the repository was still around from a previous run.

For extensions, there is an extension_load.txt file, so we can keep the extensions between runs, but for skins there is no such things and I guess the skins are autoloaded.

So we need the job to clean up the skin dir.

Event Timeline

hashar created this task.Sep 26 2015, 11:44 AM
hashar raised the priority of this task from to Needs Triage.
hashar updated the task description. (Show Details)
hashar added subscribers: hashar, Paladox.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 26 2015, 11:44 AM

Well I created this patch https://gerrit.wikimedia.org/r/#/c/228470/ that allows skins to also be tested.

And the installer magically install them in the LocalSettings.php

## Default skin: you can change the default skin. Use the internal symbolic
## names, ie 'vector', 'monobook':
$wgDefaultSkin = "metrolook";

# Enabled skins.
# The following skins were automatically enabled:
wfLoadSkin( 'apex' );
wfLoadSkin( 'Metrolook' );

Hum I wonder why. But if they are showing errors woulden that mean the skin is having the error its own repo too.

The CLI installer definitely injects whatever it can find under /skins/.

I am not sure why we trigger the testextension job on skins. With T68926 / https://gerrit.wikimedia.org/r/#/c/233694/ I have introduced a 'skins' entry point in PHPUnit.

A patch proposed against mediawiki/skins/Metrolook does trigger the job mwext-testextension-zend which runs:

phpunit.php --testsuite extensions

So you get the appropriate classes loaded because in MediaWiki core both extensions and skins suites are roughly equivalent:

<testsuite name="skins">
    <directory>skins</directory>
    <directory>structure</directory>
    <file>suites/LessTestSuite.php</file>
</testsuite>
<testsuite name="extensions">
    <directory>structure</directory>
    <file>suites/ExtensionsTestSuite.php</file>
    <file>suites/ExtensionsParserTestSuite.php</file>
    <file>suites/LessTestSuite.php</file>
</testsuite>

So it seems to me we can generalize the testextension job and have it switch the testsuite being used based on the repository triggering the change.

We also need to clean the skins between runs.

Yes. we should follow up https://gerrit.wikimedia.org/r/#/c/228470/ which adds support for testing skins.

Maybe create a task that changes the mwext-testextension- test so that it tests both extensions or skins.

Would the error that shows in the skin with that test would that mean there is a problem or is that for extensions only and skins do something differently.

Jdforrester-WMF triaged this task as Unbreak Now! priority.Sep 26 2015, 2:51 PM
Jdforrester-WMF set Security to None.

Change 241314 had a related patch set uploaded (by Paladox):
Remove extension-unittests-generic test from skins

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

Change 241314 merged by jenkins-bot:
Remove extension-unittests-generic test from skins

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

Paladox removed a subscriber: gerritbot.
hashar lowered the priority of this task from Unbreak Now! to Normal.Sep 26 2015, 7:03 PM

I removed the skins from the slaves workspaces so it should be fine now:

salt '*-slave-*' cmd.run 'rm -fR /mnt/jenkins-workspace/workspace/*/src/skins/Metrolook'
salt '*-slave-*' cmd.run 'rm -fR /mnt/jenkins-workspace/workspace/*/src/skins/apex'

Next step, delete them before running the job and or conditionally load the skins based on the list of repos that participate in the job.

We should figure out how to make the installer not add every skin to LocalSettings.php, and instead have our own skin autoloader like we already have for extensions.

Change 270712 had a related patch set uploaded (by Paladox):
Make sure the skins directory is empty in extensions unit tests and qunit tests

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

Change 270712 abandoned by Paladox:
Make sure the skins directory is empty in extensions unit tests and qunit tests

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

I belive we can close this as resolved as we support skins as deps in extensions now.

hashar closed this task as Resolved.Dec 15 2016, 7:48 AM
hashar claimed this task.

Indeed ! ;-}