Page MenuHomePhabricator

Unit test ext.popups.core.js
Closed, ResolvedPublic

Description

Tested:

  • mw.popups.setupTriggers
  • mw.popups.getTitle
  • mw.popups.removeTooltips
  • mw.popups.selectPopupElements

Event Timeline

Restricted Application removed a project: Patch-For-Review. · View Herald TranscriptApr 19 2016, 9:27 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 283348 had a related patch set uploaded (by Jhernandez):
Add QUnit tests to cover ext.popups.core.js

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

phuedx updated the task description. (Show Details)Apr 22 2016, 8:53 AM

Change 283348 merged by jenkins-bot:
Add QUnit tests to cover ext.popups.core.js

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

Change 284873 had a related patch set uploaded (by Phuedx):
Add missing mw.popups.selectPopupElements test case

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

phuedx claimed this task.Apr 22 2016, 9:28 AM
phuedx added a subscriber: bmansurov.

Change 284876 had a related patch set uploaded (by Phuedx):
Add test to cover mw.popups.setupTriggers

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

Change 284876 abandoned by Phuedx:
Add test to cover mw.popups.setupTriggers

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

phuedx added a comment.EditedApr 22 2016, 1:38 PM

Not tested:

  • mw.popups.setupTriggers

After a few attempts at getting 284876 to pass in the build environment, I can only conclude that mw.popups.scrolled is being set to true and the "trigger handler" (event listener) is short-circuiting.

I abandoned the change as it felt awkward to override the global state or to manipulate the scroll position of the browser to ensure that the test passes.

284873 still needs review…

The above patch has been +2'ed. Are we also going to test mw.popups.setupTriggers?

Change 284873 merged by jenkins-bot:
Add missing mw.popups.selectPopupElements test case

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

@bmansurov: Could you clarify why you moved this into -1 (Needs More Work)?

Change 284876 restored by Phuedx:
Add test to cover mw.popups.setupTriggers

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

Not tested:

  • mw.popups.setupTriggers

After a few attempts at getting 284876 to pass in the build environment, I can only conclude that mw.popups.scrolled is being set to true and the "trigger handler" (event listener) is short-circuiting.
I abandoned the change as it felt awkward to override the global state or to manipulate the scroll position of the browser to ensure that the test passes.

I opted to manipulate global state… 284876 is ready for review.

Note well that the test failures are due to T133163.

Tgr added a subscriber: Tgr.Apr 27 2016, 10:36 AM

Those tests should probably call HTMLForm::loadData() before HTMLForm::getHTML(). Nevertheless, https://gerrit.wikimedia.org/r/#/c/285623/ should fix them.

phuedx removed phuedx as the assignee of this task.Apr 28 2016, 4:47 AM
phuedx closed subtask T133163: BetaFeatures test failure as Resolved.
phuedx updated the task description. (Show Details)
phuedx added a subscriber: phuedx.

Change 284876 merged by Phuedx:
Add test to cover mw.popups.setupTriggers

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

I wrote one of the changes for this task so I probably shouldn't sign this off.

@Jdlrobson would you please sign off?

@bmansurov: Could you clarify why you moved this into -1 (Needs More Work)?

As you've already figured out, mw.popups.setupTriggers was not covered at the time.