Page MenuHomePhabricator

Parser tests fail if default Skin for unit tests makes use of doEditSectionLink
Closed, ResolvedPublic

Description

The parser has various integration tests that assume the installation of Vector.

Now MinervaNeue is a skin this assumption is flawed. The doEditSectionLink hook, which is used by the MinervaNeue skin, can alter the output HTML.

The tests should be rewritten to explicitly use the FallbackSkin which does not use the doEditSectionLink and will make the tests more consistent.

This blocks merges on:

  • RelatedArticles
  • MobileApps

Replication steps

  • Install MinervaNeue and MobileFrontend make sure MinervaNeue is the only installed skin (Disable the Vector skin locally. )
  • Run > php tests/phpunit/phpunit.php --testsuite extensions
  • Wait because it's slow...

Problem

The HTML in question:

<a href="/index.php?title=API&amp;action=edit&amp;section=1" title="Edit section: foo" data-section="1" class="mw-ui-icon mw-ui-icon-element mw-ui-icon-edit-enabled edit-page">Edit</a>

is provided by SkinMinerva::doEditSectionLink.

ParserOutput calls this... https://github.com/wikimedia/mediawiki/blob/master/includes/parser/ParserOutput.php#L269

It seems our parser tests assume they are running under Vector (or another skin that does not make use of doEditSectionLink)? This looks like a bug in core's tests....

There are two places where the tests need to specify a skin:

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 17 2017, 8:48 PM

Wondering if this will fix it... https://gerrit.wikimedia.org/r/365722 .. but do not know much about how phpunit suites work.

Change 365796 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[integration/config@master] test-skins should also run testsuite extensions

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

Change 365801 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/core@master] PHPUnitTets skins testsuite should pick up skins tests

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

Change 365801 abandoned by Pmiazga:
PHPUnitTets skins testsuite should pick up skins tests

Reason:
Not required as it is possible to tell CI to execute test suite both for skin and extension. Skins testsuite executes only tests/phpunit/skins testcases, and extension testsuite executes skin specific unit tests.

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

Change 365796 merged by jenkins-bot:
[integration/config@master] test-skins should also run testsuite extensions

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

Change 365722 had a related patch set uploaded (by Krinkle; owner: Jdlrobson):
[mediawiki/core@master] phpunit: Load extension unit tests for --testsuite=skins

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

Change 365722 merged by jenkins-bot:
[mediawiki/core@master] phpunit: Load extension unit tests for --testsuite=skins

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

I'm pulling this into the sprint as it blocks T149413 and any other work relating to the Minerva skin.

I'm looking at it now, as a first step, we can mark problematic test scenarios as skipped so they do not block other work.

Jdlrobson raised the priority of this task from Normal to High.Jul 18 2017, 11:18 PM

This is impacting apps so bumping to high.

phuedx added a subscriber: phuedx.Jul 19 2017, 12:21 PM
pmiazga claimed this task.Jul 19 2017, 1:09 PM

Change 366249 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@master] Unit tests have to use global RequestContext

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

https://gerrit.wikimedia.org/r/366249 should fix ApiMobileViewTest test issues.

In standup we decided we will skip this test for the time being.

@Jdlrobson, @pmiazga: Now that rEMFR1323b8ce779a: Skip ApiMobileViewTest::testView() as it is not possible to reproduce has been merged, is this still high priority? Are the apps teams now unaffected (see T170880#3450648)?

@phuedx - there is still one error to fix - https://integration.wikimedia.org/ci/job/mwext-testextension-hhvm-jessie/11287/console - this one is bitt more difficult to fix as it occurs in Core
Change that causes this error: https://gerrit.wikimedia.org/r/#/c/365042/

We are still blocking merges in apps. Only 2 remaining failures now:

19:34:57 There were 2 failures:
19:34:57 
19:34:57 1) ParserIntegrationTest::testParse with data set "luaParserTests.txt: Scribunto: expandTemplate with headers" ('[details omitted]')
19:34:57 Failed asserting that two strings are equal.
19:34:57 --- Expected
19:34:57 +++ Actual
19:34:57 @@ @@
19:34:57 -'<h2><span class="mw-headline" id="foo">foo</span><span class="mw-editsection"><span class="mw-editsection-bracket">[</span><a href="/index.php?title=Parser_test&amp;action=edit&amp;section=1" title="Edit section: foo">edit</a><span class="mw-editsection-bracket">]</span></span></h2>
19:34:57 -<h2><span class="mw-headline" id="bar">bar</span><span class="mw-editsection"><span class="mw-editsection-bracket">[</span><a href="/index.php?title=Template:Scribunto_template_with_headers&amp;action=edit&amp;section=T-1" title="Template:Scribunto template with headers">edit</a><span class="mw-editsection-bracket">]</span></span></h2>
19:34:57 +'<h2><span class="mw-headline" id="foo">foo</span><span><a href="/index.php?title=API&amp;action=edit&amp;section=1" title="Edit section: foo" data-section="1" class="mw-ui-icon mw-ui-icon-element mw-ui-icon-edit-enabled edit-page">Edit</a></span></h2>
19:34:57 +<h2><span class="mw-headline" id="bar">bar</span><span><a href="/index.php?title=API&amp;action=edit&amp;section=T-1" title="Edit section: " data-section="T-1" class="mw-ui-icon mw-ui-icon-element mw-ui-icon-edit-enabled edit-page">Edit</a></span></h2>
19:34:57  '
19:34:57 
19:34:57 /home/jenkins/workspace/mwext-testextension-hhvm-jessie/src/tests/phpunit/includes/parser/ParserIntegrationTest.php:37
19:34:57 /home/jenkins/workspace/mwext-testextension-hhvm-jessie/src/maintenance/doMaintenance.php:111
19:34:57 
19:34:57 2) ParserIntegrationTest::testParse with data set "citeParserTests.txt: <ref>s with the follow parameter" ('[details omitted]')
19:34:57 Failed asserting that two strings are equal.
19:34:57 --- Expected
19:34:57 +++ Actual
19:34:57 @@ @@
19:34:57  '<p>Page one.<sup id="cite_ref-beginning_1-0" class="reference"><a href="#cite_note-beginning-1">&#91;1&#93;</a></sup>
19:34:57  </p><p>Page two.
19:34:57  </p>
19:34:57 -<h2><span class="mw-headline" id="References">References</span><span class="mw-editsection"><span class="mw-editsection-bracket">[</span><a href="/index.php?title=Parser_test&amp;action=edit&amp;section=1" title="Edit section: References">edit</a><span class="mw-editsection-bracket">]</span></span></h2>
19:34:57 +<h2><span class="mw-headline" id="References">References</span><span><a href="/index.php?title=API&amp;action=edit&amp;section=1" title="Edit section: References" data-section="1" class="mw-ui-icon mw-ui-icon-element mw-ui-icon-edit-enabled edit-page">Edit</a></span></h2>
19:34:57  <div class="mw-references-wrap"><ol class="references">
19:34:57  <li id="cite_note-beginning-1"><span class="mw-cite-backlink"><a href="#cite_ref-beginning_1-0">↑</a></span> <span class="reference-text">First page footnote text. Second page footnote text.</span>
19:34:57  </li>
19:34:57  </ol></div>
19:34:57  '

Looks like the remaining issue is a Scribunto test failure. Looks like the MobileFormatter is running unexpectedly on the result...

Tests in question are:
https://github.com/wikimedia/mediawiki-extensions-Scribunto/blob/31820c673a9caa4cec8fdb3452b32aa82fe0614f/tests/phpunit/engines/LuaCommon/luaParserTests.txt

This issue never showed up prior to separating MobileFrontend and Minerva and is now showing up (and blocking merges) on patchsets in #MobileApps RelatedArticles and MinervaNeue

I'm not sure why MobileFormatter is running in this context. It should only ever run via a hook if using a mobile user agent.

@Legoktm any idea what's going on ?

Anomie added a subscriber: Anomie.

This does not seem to have anything to do with Scribunto itself. Your skin/extension is screwing up the section edit links somehow, making parser tests registered by Scribunto and by Cite fail.

I don't know why you're not causing core's parser tests to fail too, unless maybe you're only running extension tests and not core tests.

This does not seem to have anything to do with Scribunto itself.

@Anomie yes I know that but somehow that test is failing here and might be failing for other extensions so I'm doing my best to communicate this problem to others who may be experiencing it.

Your skin/extension is screwing up the section edit links somehow, making parser tests registered by Scribunto and by Cite fail.

The MobileFrontend extension massages edit links to make them mobile friendly but it should not be running in a test context unless something is explicitly turning it on. Something strange is definitely going on in CI config as this test passes in MobileFrontend extension. Hoping someone who understands the CI stack a bit better can shed some light. Could this be related to the mw-test-skin job we setup last week?

Mm... after debugging this some more I think I know the issue.

The HTML in question:

<a href="/index.php?title=API&amp;action=edit&amp;section=1" title="Edit section: foo" data-section="1" class="mw-ui-icon mw-ui-icon-element mw-ui-icon-edit-enabled edit-page">Edit</a>

is provided by SkinMinerva::doEditSectionLink.

ParserOutput calls this... https://github.com/wikimedia/mediawiki/blob/master/includes/parser/ParserOutput.php#L269

It seems our parser tests assume they are running under Vector (or another skin that does not make use of doEditSectionLink)? This looks like a bug in core's tests....

Jdlrobson renamed this task from Not possible for a skin to have PHPUnit tests (PHPunit tests for skin do not seem to be run) to Parser tests fail if default Skin for unit tests makes use of doEditSectionLink.Jul 19 2017, 9:34 PM

Change 366470 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[integration/config@master] Include Vector in phpunit tests for MobileFrontend

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

^ short term solution..? (untested)
I'm hopeful that will get Jenkins green for https://gerrit.wikimedia.org/r/365042 ?

cc @hashar your help here would be appreciated!

Jdlrobson added subscribers: tstarling, Krinkle.

The correct way to fix this would be something like this:

But I have no knowledge of how these tests are structured. Maybe @Krinkle @tstarling or @Legoktm know.

Change 366496 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/core@master] parserTests: Use "fallback" skin unless otherwise specified

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

@Jdlrobson I basically did the same concept as you, but left it configurable since we probably do want to have parser tests to verify the output of MinervaNeue's edit section links.

Legoktm added a comment.EditedJul 20 2017, 1:47 AM

(Also whenever I would try testing it with php parserTests.php I'd get fatal exceptions about $this->getTitle() not being set which is a bit weird, but it made it obvious whether MinervaNeue was being used to parse or not.)

Jdlrobson added a comment.EditedJul 20 2017, 5:25 AM

@Legoktm you are awesome! :)

Change 366470 abandoned by Jdlrobson:
Include Vector in phpunit tests for MobileFrontend

Reason:
not sure this helps

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

Change 366988 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Parser on skins other than DefaultSkins

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

Jdlrobson removed pmiazga as the assignee of this task.Jul 24 2017, 4:58 PM

Tji

Jdlrobson updated the task description. (Show Details)

I will visit this task later on this week. Came here to mention I have hit a similar wall with thumbnailBeforeProduceHTML hook which change the output expected by core or other extensions. T69302

Change 366988 abandoned by Krinkle:
DONOTMERGE: Parser on skins other than DefaultSkins

Reason:
Fixed by Ib7f0bd15dd0a92

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

The fix is https://gerrit.wikimedia.org/r/#/c/366496/ by @Legoktm

If any changes are blocked on this issue, one can add to their changes:

Depends-On: Ib7f0bd15dd0a9255e1e5140907e800478b658b92

And that the tests should pass (assuming the parser tests are written based on the Fallback skin or mention in their options: skin=Vector

Change 366496 merged by jenkins-bot:
[mediawiki/core@master] parserTests: Use "fallback" skin unless otherwise specified

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

Krinkle closed this task as Resolved.Jul 26 2017, 1:13 AM
Krinkle removed a project: Patch-For-Review.
Krinkle assigned this task to Legoktm.