Page MenuHomePhabricator

ExtractFormatterTest are broken
Closed, ResolvedPublic2 Story Points

Description

Might be a regression. Need to investigate what caused the change here (it seems maybe a title attribute moved into its own element).

16:42:31 1) ExtractFormatterTest::testExtracts with data set #0 ('Dutch ( Nederlands ) is a Wes...rlands', ''''Dutch''' (<span class="uni...ands]]', true)
16:42:31 Failed asserting that two strings are equal.
16:42:31 --- Expected
16:42:31 +++ Actual
16:42:31 @@ @@
16:42:31 -'Dutch ( Nederlands ) is a West Germanic language and the native language of most of the population of the Netherlands'
16:42:31 +'Dutch (About this sound Nederlands ) is a West Germanic language and the native language of most of the population of the Netherlands'
16:42:31 
16:42:31 /mnt/jenkins-workspace/workspace/mwext-testextension-php55/src/extensions/TextExtracts/tests/phpunit/ExtractFormatterTest.php:21
16:42:31 /mnt/jenkins-workspace/workspace/mwext-testextension-php55/src/tests/phpunit/MediaWikiTestCase.php:389

Event Timeline

Reedy created this task.Aug 5 2016, 5:27 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 5 2016, 5:27 PM
Jdlrobson triaged this task as High priority.Aug 8 2016, 5:17 PM

When the time comes, this will be for engineer signoff.

MBinder_WMF set the point value for this task to 2.Aug 10 2016, 4:23 PM
MBinder_WMF moved this task from Needs Analysis to To Do on the Reading-Web-Sprint-79-Uh-oh board.
jhobs updated the task description. (Show Details)Aug 10 2016, 4:24 PM

I can replicate this.
The test assumes [[File:Loudspeaker]] exists on the wiki.

Change 304887 had a related patch set uploaded (by Jdlrobson):
ExtractFormatter should not test the Parser

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

Looking at this ExtractFormatterTest uses Parser which will have different results depending on the state of the wiki at execution.

I'm not sure why since ExtractFormatter runs on the resulting HTML. The fix I've posted removes the Parser step. That said I can't work out what the tests below it are testing before and whether this fix breaks them in some way. Does anyone understand them?

Someone should feel free to take over ownership of this task if they know a better way to fix this.

Jdlrobson added a subscriber: phuedx.

There is discussion on the task. @phuedx would in particular appreciate your thoughts there.

These tests were enabled in a5ab220. They were previously disabled due to "Jenkins weirdness".

phuedx claimed this task.Aug 16 2016, 2:11 PM

^ This isn't me saying "I know a better way!". This is me saying "I'm having a look."

phuedx added a comment.EditedAug 16 2016, 2:58 PM

I can replicate this.
The test assumes [[File:Loudspeaker]] exists on the wiki.

Thanks for this pointer @Jdlrobson. I can reproduce this failure locally by $wgUseInstantCommons set to false.

Edit

I'm using MediaWiki-Vagrant@a8e48b9.

d29293e demonstrates that these tests pass in the Jenkins build environment just fine. Could this be an ordering issue?

phuedx added a comment.EditedAug 18 2016, 1:33 PM

Here's what I know:

  • The test passes locally – using MediaWiki-Vagrant (c694667) – on the master branch
  • The test fails locally when $wgUseInstantCommons is set to false
  • The test passes on the master branch in Jenkins, e.g. d29293e
  • The test fails in Jenkins when the extension's tests are auto-discovered, e.g. 03001f0
  • The tests have been disabled for some time due to "Jenkins weirdness" (from a5ab220)
phuedx added a subscriber: MaxSem.Aug 18 2016, 1:38 PM

@Jdlrobson: I'm not sure how to proceed here. As I've mentioned previously, I may simply disable the test (again) /cc @MaxSem

Change 305541 had a related patch set uploaded (by Phuedx):
Disable integration-like testExtracts test

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

Jdlrobson reassigned this task from phuedx to MaxSem.Aug 18 2016, 6:52 PM

@phuedx disabling the test seems worse than repurposing it.

FWIW I don't agree with @MaxSem's review on https://gerrit.wikimedia.org/r/#/c/304887/
The test is clearly an integration test masked as a unit test and I don't see its value in the current form.

I think it's on @MaxSem to argue its existence and provide a better solution or to remove his -1.

Jdlrobson removed MaxSem as the assignee of this task.Aug 22 2016, 5:06 PM
phuedx added a comment.EditedAug 23 2016, 8:58 AM

That said I can't work out what the tests below it are testing before and whether this fix breaks them in some way. Does anyone understand them?

Those tests are verifying that class and style attributes are removed. (Hint: the third element of the array is the fixture.)

Change 304887 merged by jenkins-bot:
ExtractFormatter should not test the Parser

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

Change 305541 abandoned by Phuedx:
Disable integration-like testExtracts test

Reason:
See T142230 for detail.

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

phuedx closed this task as Resolved.Aug 24 2016, 7:07 AM
phuedx claimed this task.

ad435fb4 was rebased and merged.