Page MenuHomePhabricator

[EPIC] Hovercards: Test Coverage
Closed, ResolvedPublic

Description

Test coverage is very low. What needs to be set up and tested?

See subtasks for work.

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 3:14 AM
bzimport added a project: Page-Previews.
bzimport set Reference to bz65103.
bzimport added a subscriber: Unknown Object (MLST).
Jaredzimmerman-WMF updated the task description. (Show Details)
Jaredzimmerman-WMF set Security to None.

Change 179106 had a related patch set uploaded (by Prtksxna):
tests: Add render.article.createImgThumbnail

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

Patch-For-Review

Change 179106 abandoned by Prtksxna:
tests: Add render.article.createImgThumbnail

Reason:
I think I am doing something utterly useless here. The code in the test is the same as the function.

Krinkle, does this make sense?

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

Jdlrobson lowered the priority of this task from Medium to Low.Dec 17 2015, 8:28 PM
Jdlrobson subscribed.
dr0ptp4kt renamed this task from Hovercards: Write Tests to Hovercards: Test Coverage.Apr 4 2016, 4:41 PM
dr0ptp4kt updated the task description. (Show Details)

OK, here is my analysis so far:

1. ext.popups.core.js

Not tested:

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

Tested:

  • mw.popups.getTitle

2. ext.popups.renderer.article.js

Not tested:

  • article.createPopup
  • article.removeParensFromText
  • article.createSVGTag
  • article.createThumbnail
  • article.createSvgImageThumbnail
  • article.createImgThumbnail
  • article.getOffset
  • article.getClasses
  • article.processPopup

Tested:

  • article.getProcessedElements
  • mw.popups.render.getClosestYPosition

3. These files are not tested: ext.popups.disablenavpop.js, ext.popups.logger.js, ext.popups.settings.js.

I think this task is definitely more than a 3 pointer (probably an 8) because the extension is covered sparsely.

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

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

We should take care when doing this that we write the write tests. Browser tests may be better in some places and there may not be value in testing certain units.

phuedx subscribed.

… as this was brought into the sprint after it began.

Change 283566 had a related patch set uploaded (by Bmansurov):
Add basic browser tests

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

Change 283568 had a related patch set uploaded (by Bmansurov):
Create Popups jenkins job

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

Change 283762 had a related patch set uploaded (by Bmansurov):
Add QUnit tests for ext.popups.logger

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

I'm taking this back to transform it to an Epic and get it better fleshed out and split to pieces.

The spike already done and other work we need to do taking into account the spike's results.

@bmansurov I'll split tasks for the patches you already have, and change the patches to link to the correct bugs. We can talk tomorrow if there is something wrong.

jhobs renamed this task from Hovercards: Test Coverage to [EPIC] Hovercards: Test Coverage.Jun 1 2016, 9:50 PM
jhobs removed a project: Unplanned-Sprint-Work.
jhobs moved this task from 2015-16 Q4 to 2014-15 Q4 on the Web-Team-Backlog board.
Jdlrobson claimed this task.

The repo is pretty well tested now. Remaining work is tracked in T133022 and T133162