Page MenuHomePhabricator

Move browser test settings for CirrusSearch on MediaWiki-Vagrant from Jenkins.php to a separate CirrusSearch test wiki (using mediawiki::wiki).
Closed, ResolvedPublic

Description

This is the second time I've found an odd setting coming from CirrusSearch's tests/jenkins/Jenkins.php .

If this needs to load for browser tests and/or Jenkins, it should only load for Jenkins. Loading it for normal MediaWiki-Vagrant usage interferes with completely different things.

I was researching the effects of the Researcher group, and then I found out *all* users somehow had deleterevision (usually granted to admin-type users). After grepping through all MediaWiki-Vagrant extensions, I discovered it was coming from

$wgGroupPermissions[ '*' ][ 'deleterevision' ] = true;

in CirrusSearch.

This is just an example, though. This file just should only be loaded when actually necessary.

This has been discussed before at T73778: CirrusSearch tests should be able to pass with wgCapitalLinks = true and https://gerrit.wikimedia.org/r/#/c/155861/ , but without a solution thus far.

Event Timeline

Mattflaschen-WMF updated the task description. (Show Details)
Mattflaschen-WMF raised the priority of this task from to Needs Triage.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 21 2015, 2:42 AM
demon triaged this task as Lowest priority.Mar 12 2015, 10:08 PM

Jenkins.php is a really bad name for it. Right now we just dump everything required for browser tests or to setup the wiki for cirrus into the same file. It should be two files. In fact @bd808 had started the work in vagrant to make a browsertest role for cirrus but that process was torpedoed. I'm happy to reopen the effort but I don't recall why it wasn't ok.

Jenkins.php is a really bad name for it. Right now we just dump everything required for browser tests or to setup the wiki for cirrus into the same file. It should be two files. In fact @bd808 had started the work in vagrant to make a browsertest role for cirrus but that process was torpedoed. I'm happy to reopen the effort but I don't recall why it wasn't ok.

@MaxSem was actually trying to break things up, but @dduvall wanted tests to work "out of the box" everywhere. I'm not sure what the right thing to do here really is, but the fact that the browser tests require configuration that breaks wikis otherwise seems problematic for keeping things in a single role. Maybe we could make it easier to disable the loading of the test config? Or maybe this is a case where best for most wins out since I don't imagine that most users are interested in running CirrusSearch tests?

@MaxSem was actually trying to break things up, but @dduvall wanted tests to work "out of the box" everywhere.

IIRC, the main concern regarding browser tests at the time was the unholy provisioning cost. We've since "solved" (mitigated) that problem and I don't hear many complaints about it anymore.

Maybe I'm wrong but this seems like a similar scenario where we're looking to sweep a real issue under the (role) rug. Can't we instead just create a user/group specifically for cirrussearch testing and give deleterevision to that? I can help with the test refactoring (mediawiki_selenium 1.0.0 supports configuration for all sorts of things including multiple users).

Also, if refactoring the tests and configuration is really too problematic, we can always implement a role parameter instead of completely splitting the role.

Maybe I'm wrong but this seems like a similar scenario where we're looking to sweep a real issue under the (role) rug. Can't we instead just create a user/group specifically for cirrussearch testing and give deleterevision to that? I can help with the test refactoring (mediawiki_selenium 1.0.0 supports configuration for all sorts of things including multiple users).

It wouldn't be enough. Cirrus needs a wiki that doesn't have capital links to test wiktionary stuffs and that setting is far more disruptive. It might make sense to just have cirrus create a new wiki with those settings for browser testing. Cirrus's testing predates that being a reasonable thing to do in vagrant.

bd808 added a comment.Mar 13 2015, 1:26 AM

It wouldn't be enough. Cirrus needs a wiki that doesn't have capital links to test wiktionary stuffs and that setting is far more disruptive. It might make sense to just have cirrus create a new wiki with those settings for browser testing. Cirrus's testing predates that being a reasonable thing to do in vagrant.

Setting up a test wiki (or 2 or 3) would be easy in the current setup and nearly free from a resources standpoint.

Mattflaschen-WMF renamed this task from Jenkins.php should not be loaded on normal page loads on MediaWiki-Vagrant to Move browser test settings for Cirruso on MediaWiki-Vagrant from Jenkins.php to a separate CirrusSearch test wiki (using mediawiki::wiki)..Mar 25 2015, 1:58 AM
Mattflaschen-WMF renamed this task from Move browser test settings for Cirruso on MediaWiki-Vagrant from Jenkins.php to a separate CirrusSearch test wiki (using mediawiki::wiki). to Move browser test settings for CirrusSearch on MediaWiki-Vagrant from Jenkins.php to a separate CirrusSearch test wiki (using mediawiki::wiki)..

Change 205896 had a related patch set uploaded (by EBernhardson):
Move cirrusearch browsertest config to cirrustestwiki

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

Change 206012 had a related patch set uploaded (by EBernhardson):
Update browsertest environment for vagrant changes

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

Change 205896 merged by jenkins-bot:
Move cirrusearch browsertest config to cirrustestwiki

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

Change 206012 merged by jenkins-bot:
Update browsertest environment for vagrant changes

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

Mattflaschen-WMF closed this task as Resolved.Apr 25 2015, 4:19 AM

Thanks. :) This was a recurring point point for me (and always took digging to find the source).

bd808 moved this task from Backlog to Done on the MediaWiki-Vagrant board.May 4 2015, 10:45 PM
Restricted Application added a project: Discovery. · View Herald TranscriptDec 31 2015, 5:10 AM