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

Related Gerrit Patches:

mediawiki/extensions/BetaFeatures : master	Fix failing BetaFeatures unit tests by using prepareForm
mediawiki/core : master	Enforce calling HTMLForm::prepareForm before displayForm
mediawiki/core : master	Handle null data return in HTMLForm
mediawiki/extensions/BetaFeatures : master	NewHTMLCheckField fixes
mediawiki/core : master	Make HTMLCheckField::loadDataFromRequest always return a boolean
mediawiki/extensions/BetaFeatures : master	Correction to test

Related Objects
Search...

Status	Assigned	Task
Declined	Jdlrobson	T111329 [GOAL] Page previews on mobileweb
Resolved	phuedx	T133947 [EPIC] Reduce bloat of Page Previews RL module(s)
Resolved	Jdlrobson	T144522 [EPIC] Hovercards technical debt and quality enhancements
Resolved	Jdlrobson	T67103 [EPIC] Hovercards: Test Coverage
Resolved	Jdlrobson	T133020 Unit test ext.popups.core.js
Invalid	None	T133055 Split ext.popups.settings.js in units
Resolved	phuedx	T133025 Unit test ext.popups.settings.js
Resolved	phuedx	T133054 Create browser test for add footer link in ext.popups.settings.js
Resolved	Jdlrobson	T133163 BetaFeatures test failure

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.

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

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

Change 284485 merged by jenkins-bot:
Correction to test

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

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.

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.

Yeah, apparently we only run tests for these extensions.

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

gerritbot added a project: Patch-For-Review.Apr 23 2016, 4:25 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

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

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

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

ReleaseTaggerBot added a project: MW-1.27-release (WMF-deploy-2016-05-03_(1.27.0-wmf.23)).Apr 26 2016, 3:01 PM

Thanks @Tgr for investigating this and getting this fixed!

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

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

@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?

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

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.

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

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

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.

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.

In T133163#2246423, @phuedx wrote:

@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…

BetaFeatures test failureClosed, ResolvedPublicActions

Description

Details

Related ObjectsSearch...

Event Timeline

BetaFeatures test failure
Closed, ResolvedPublic
Actions

Related Objects
Search...