Page MenuHomePhabricator

Extension unit tests should not depend on VisualEditor (Cite/Citoid/Graph)
Closed, ResolvedPublic1 Story Points

Description

Cite uses ext.visualEditor.mwcore as of I39936ed83d5a60471a0a75da753f498e80aef234
There are many Mediawiki installs without VisualEditor (my local wiki included)
Please make this a soft dependency and register modules only when VisualEditor is enabled rather than breaking those instances.

Event Timeline

Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald TranscriptFeb 22 2016, 10:22 PM

FYI this is really messing with my developer workflow since the reading team is doing lots of Cite development right now. I personally have problems getting VisualEditor installed outside vagrant - which I try to avoid using since it is super slow. I would thus appreciate this being given attention - it may seem small and a low priority but it is giving me lots of problems and I imagine others too.

FYI this is really messing with my developer workflow since the reading team is doing lots of Cite development right now. I personally have problems getting VisualEditor installed outside vagrant - which I try to avoid using since it is super slow. I would thus appreciate this being given attention - it may seem small and a low priority but it is giving me lots of problems and I imagine others too.

As a short-term hack, you can just not run those unit tests. Cite doesn't require VE to run, only "ext.cite.visualEditor.core" etc. do.

MobileFrontend has lots of soft dependencies and doesn't break when VisualEditor is not installed for example. It's trivial to add these modules conditionally via a hook in MobileFrontend after checking for the presence of the soft dependency.
e.g. https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/includes/MobileFrontend.hooks.php#L1027

Is there a good reason this method cannot be used in Cite?

I'm using short term hacks but they are a pain.

Is there a good reason this method cannot be used in Cite?

Well, yes – we're getting rid of those hooks intentionally for performance. That's part of the reason the VE half of the code moved into this repo, so the cross-repo dependency was on the same level as for other extensions.

Although performance is a noble cause, you are breaking expectations of how dependencies work and breaking these extensions for 3rd parties.

This change makes Cite and Geshi depend on VisualEditor. If this is going to be the status quo for some time it should be reflected on the extension pages.

What solution are you offering for treating soft dependencies in future?
How can I help remedy this?

Please don't CC entire departments unless it's actually very relevant to the entire department.

Editing-Department is not just the VE team (I'll let them comment on when it's worth CCing the whole VE team).

This makes it a pain to unsubscribe without unintentionally unsubscribing other people who are part of that department but never individually subscribed.

Esanders renamed this task from Cite should not depend on VisualEditor to Cite unit tests should not depend on VisualEditor.Mar 1 2016, 11:49 AM
Esanders renamed this task from Cite unit tests should not depend on VisualEditor to Extension unit tests should not depend on VisualEditor (Cite/Citoid/Graph).Mar 1 2016, 12:19 PM

Change 274091 had a related patch set uploaded (by Esanders):
Check VE is available before running unit tests

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

Change 274092 had a related patch set uploaded (by Esanders):
Check VE is available before running unit tests

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

Change 274093 had a related patch set uploaded (by Esanders):
Check VE is available before running unit tests

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

Graph and Cite were checking the wrong module to test for VE existence. Citoid had no check yet.

Jdforrester-WMF closed this task as Resolved.Mar 1 2016, 3:59 PM
Jdforrester-WMF triaged this task as Normal priority.
Jdforrester-WMF assigned this task to Esanders.
Jdforrester-WMF edited projects, added Citoid, Graphs, Technical-Debt; removed Patch-For-Review.
Jdforrester-WMF set the point value for this task to 1.

Change 274093 merged by jenkins-bot:
Check VE is available before running unit tests

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

Change 274092 merged by jenkins-bot:
Check VE is available before running unit tests

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

Change 274091 merged by jenkins-bot:
Check VE is available before running unit tests

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