Page MenuHomePhabricator

BetaFeatures test failure
Closed, ResolvedPublic

Description

NewCheckFieldTest::testCreatingFieldGivesExpectedStrings is failing
https://integration.wikimedia.org/ci/job/mwext-testextension-php55/8375/console

This is blocking merges on the Popups extension

Event Timeline

Restricted Application added subscribers: TerraCodes, Aklapper. · View Herald TranscriptApr 20 2016, 4:04 PM

This was broken by https://gerrit.wikimedia.org/r/#/c/280884/4
Seems this fix surfaced an error in the test...

Change 284485 had a related patch set uploaded (by Jdlrobson):
Correction to test

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

phuedx triaged this task as Unbreak Now! priority.Apr 20 2016, 5:04 PM
Restricted Application added subscribers: Luke081515, Urbanecm. · View Herald TranscriptApr 20 2016, 5:04 PM

Change 284485 merged by jenkins-bot:
Correction to test

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

Tgr claimed this task.Apr 20 2016, 5:36 PM
Tgr added a subscriber: Tgr.

Merged the patch as this was blocking work and the test does seem wrong, but this needs following up.

  • why did the tests for the core patch not fail? I thought we run the tests for all WMF-deployed extensions on core changes.
  • it seems NewHTMLCheckField set the field data to null, which fails the isset test. That seems like wrong behavior from both NewHTMLCheckField (it should use booleans) and https://gerrit.wikimedia.org/r/#/c/280884 (it should not assume null is an impossible value)
Jdlrobson lowered the priority of this task from Unbreak Now! to Normal.Apr 20 2016, 7:57 PM
Jdlrobson removed a project: Patch-For-Review.
  • why did the tests for the core patch not fail? I thought we run the tests for all WMF-deployed extensions on core changes.

We don't.

Tgr added a comment.Apr 21 2016, 10:34 AM

Yeah, apparently we only run tests for these extensions.

Tgr added a comment.EditedApr 23 2016, 4:21 PM

So the real problem was that the test never called $form->loadData(). Before https://gerrit.wikimedia.org/r/#/c/280884/ that resulted in treating all field values as null, whether or not the field had a default value; after that patch the default value is used. (Not calling loadData is an error so the change does not affect correct uses of HTMLForm.) The assertions simply codified the wrong result.

Change 285011 had a related patch set uploaded (by Gergő Tisza):
NewHTMLCheckField fixes

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

Change 285012 had a related patch set uploaded (by Gergő Tisza):
Make HTMLCheckField::loadDataFromRequest always return a boolean

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

Tgr added a comment.Apr 23 2016, 4:29 PM

Also, PHP sadness of the week award goes to array access handling which quietly returns null if the accessed variable is not an array, even in strict mode.

Change 285013 had a related patch set uploaded (by Gergő Tisza):
Handle null data return in HTMLForm

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

Change 285012 merged by jenkins-bot:
Make HTMLCheckField::loadDataFromRequest always return a boolean

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

Change 285011 merged by jenkins-bot:
NewHTMLCheckField fixes

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

Change 285013 merged by jenkins-bot:
Handle null data return in HTMLForm

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

Jdlrobson closed this task as Resolved.Apr 26 2016, 5:37 PM

Thanks @Tgr for investigating this and getting this fixed!

Change 285623 had a related patch set uploaded (by Gergő Tisza):
Make sure HTMLForm::$mFieldData is always an array

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

phuedx added a subscriber: phuedx.EditedApr 27 2016, 12:57 PM

@Tgr: An alternative to 286623 would be to have HTMLForm#getHTML() throw a LogicException when it's called before #loadData as it seems that the latter must be called before the former is.

What do you think?

Tgr added a comment.Apr 27 2016, 2:30 PM

Yeah, probably a better approach. I'll update the patch.

Zppix added a subscriber: Zppix.Apr 27 2016, 2:37 PM

I will review code asap

Change 285623 merged by jenkins-bot:
Enforce calling HTMLForm::prepareForm before displayForm

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

BetaFeatures tests are still failing but now it is much more explicit why.

Change 285764 had a related patch set uploaded (by Jdlrobson):
Fix failing BetaFeatures unit tests by using prepareForm

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

Change 285764 merged by jenkins-bot:
Fix failing BetaFeatures unit tests by using prepareForm

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

@Tgr and @Jdlrobson's changes are merged, which allowed the relevant change against #mediawiki-extensions-Popups (284873: Add missing mw.popups.selectPopupElements test case) to be merged.

@Tgr and @Jdlrobson's changes are merged, which allowed the relevant change against #mediawiki-extensions-Popups (284873: Add missing mw.popups.selectPopupElements test case) to be merged.

284876: Add test to cover mw.popups.setupTriggers even…