Page MenuHomePhabricator
Create Task
Maniphest T133163

BetaFeatures test failure
Closed, ResolvedPublic
Actions

Description

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

This is blocking merges on the Popups extension

Details

ProjectBranchLines +/-Subject
mediawiki/extensions/BetaFeaturesmaster+3 -0Fix failing BetaFeatures unit tests by using prepareForm
mediawiki/coremaster+31 -1Enforce calling HTMLForm::prepareForm before displayForm
mediawiki/coremaster+5 -5Handle null data return in HTMLForm
mediawiki/extensions/BetaFeaturesmaster+4 -7NewHTMLCheckField fixes
mediawiki/coremaster+2 -2Make HTMLCheckField::loadDataFromRequest always return a boolean
mediawiki/extensions/BetaFeaturesmaster+1 -1Correction to test
Customize query in gerrit

Related Objects
Search...

Event Timeline

Jdlrobson created this task.Apr 20 2016, 4:04 PM
Restricted Application added subscribers: TerraCodes, Aklapper. · View Herald TranscriptApr 20 2016, 4:04 PM
Jdlrobson added projects: Reading-Web-Sprint-70-Lady-and-the-Trumps, Unplanned-Sprint-Work.Apr 20 2016, 4:22 PM
Jdlrobson claimed this task.Apr 20 2016, 4:27 PM
Jdlrobson moved this task from Needs Analysis to Code Review on the Reading-Web-Sprint-70-Lady-and-the-Trumps board.
Jdlrobson moved this task from Code Review to Doing on the Reading-Web-Sprint-70-Lady-and-the-Trumps board.
Jdlrobson added a comment.Apr 20 2016, 4:46 PM

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

gerritbot added a subscriber: gerritbot.Apr 20 2016, 4:53 PM

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

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

gerritbot added a project: Patch-For-Review.Apr 20 2016, 4:53 PM
Jdlrobson moved this task from Doing to Code Review on the Reading-Web-Sprint-70-Lady-and-the-Trumps board.Apr 20 2016, 4:56 PM
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
gerritbot added a comment.Apr 20 2016, 5:27 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 moved this task from Code Review to Done on the Reading-Web-Sprint-70-Lady-and-the-Trumps board.Apr 20 2016, 5:40 PM
ReleaseTaggerBot added a project: MW-1.27-release (WMF-deploy-2016-04-26_(1.27.0-wmf.22)).Apr 20 2016, 6:00 PM
Jdlrobson lowered the priority of this task from Unbreak Now! to Medium.Apr 20 2016, 7:57 PM
Jdlrobson removed a project: Patch-For-Review.
Legoktm added a subscriber: Legoktm.Apr 21 2016, 1:21 AM
In T133163#2224363, @Tgr wrote:
  • 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.

gerritbot added a comment.Apr 23 2016, 4:25 PM

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

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

gerritbot added a project: Patch-For-Review.Apr 23 2016, 4:25 PM
gerritbot added a comment.Apr 23 2016, 4:28 PM

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.

gerritbot added a comment.Apr 23 2016, 4:40 PM

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

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

gerritbot added a comment.Apr 23 2016, 5:01 PM

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

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

Zppix added a subscriber: Contributors-Team.Apr 23 2016, 5:44 PM
ReleaseTaggerBot added a project: MW-1.27-release-notes.Apr 23 2016, 6:00 PM
Krenair removed a subscriber: Contributors-Team.Apr 23 2016, 6:33 PM
gerritbot added a comment.Apr 25 2016, 7:34 AM

Change 285011 merged by jenkins-bot:
NewHTMLCheckField fixes

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

gerritbot added a comment.Apr 26 2016, 2:37 PM

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

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

ReleaseTaggerBot added a project: MW-1.27-release (WMF-deploy-2016-05-03_(1.27.0-wmf.23)).Apr 26 2016, 3:01 PM
Jdlrobson closed this task as Resolved.Apr 26 2016, 5:37 PM

Thanks @Tgr for investigating this and getting this fixed!

Jdlrobson reopened this task as Open.Apr 27 2016, 12:10 AM

Seems to have resurfaced...! :-O
https://integration.wikimedia.org/ci/job/mwext-testextension-hhvm/10496/console

Jdlrobson removed a project: Patch-For-Review.Apr 27 2016, 12:10 AM
Jdlrobson mentioned this in T133054: Create browser test for add footer link in ext.popups.settings.js.
phuedx added a parent task: T133020: Unit test ext.popups.core.js.Apr 27 2016, 9:28 AM
phuedx mentioned this in T133020: Unit test ext.popups.core.js.Apr 27 2016, 9:31 AM
gerritbot added a comment.Apr 27 2016, 10:31 AM

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

gerritbot added a project: Patch-For-Review.Apr 27 2016, 10:31 AM
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

gerritbot added a comment.Apr 27 2016, 8:19 PM

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

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

Jdlrobson removed a project: Patch-For-Review.Apr 27 2016, 8:41 PM

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

Jdlrobson added a parent task: T133054: Create browser test for add footer link in ext.popups.settings.js.Apr 27 2016, 8:41 PM
Jdlrobson added a project: Reading-Web-Sprint-71-Matisse-Monet-Kandinsky-and-the-Departing-Painters.Apr 27 2016, 10:28 PM
Jdlrobson moved this task from Needs Analysis to Doing on the Reading-Web-Sprint-71-Matisse-Monet-Kandinsky-and-the-Departing-Painters board.
Jdlrobson claimed this task.Apr 27 2016, 10:30 PM
gerritbot added a comment.Apr 27 2016, 10:31 PM

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

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

gerritbot added a project: Patch-For-Review.Apr 27 2016, 10:31 PM
Jdlrobson moved this task from Doing to Code Review on the Reading-Web-Sprint-71-Matisse-Monet-Kandinsky-and-the-Departing-Painters board.Apr 27 2016, 11:15 PM
gerritbot added a comment.Apr 28 2016, 1:21 AM

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

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

phuedx added a comment.Apr 28 2016, 4:47 AM

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

phuedx closed this task as Resolved.Apr 28 2016, 4:47 AM
phuedx moved this task from Code Review to Done on the Reading-Web-Sprint-71-Matisse-Monet-Kandinsky-and-the-Departing-Painters board.
phuedx added a comment.Apr 28 2016, 4:58 AM
In T133163#2246423, @phuedx wrote:

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

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

Content licensed under Creative Commons Attribution-ShareAlike 3.0 (CC-BY-SA) unless otherwise noted; code licensed under GNU General Public License (GPL) or other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL