Page MenuHomePhabricator

Improve test coverage for Special:Investigate
Closed, ResolvedPublic3 Estimated Story Points


Ahead of enabling this feature in production, we should make sure we have good test coverage. In particular, we could have better coverage of the pagers and the special page, but we should review the coverage of all new classes.

Acceptance criteria

  • Full coverage for the services
  • Cover regular input cases (IP, IP range, User, multiple users, and a mix)
  • Cover malformed data to make sure it fails gracefully

Event Timeline

Niharika triaged this task as Medium priority.Feb 13 2020, 6:30 PM
Niharika moved this task from Untriaged to Triage/To be Estimated on the Anti-Harassment board.

A long time ago (perhaps too long ago!) we talked about making code coverage an implied part of Anti-Harassment tasks acceptance criteria, did we abandon that?

@dbarratt We should discuss that as a team. Does it impact on whether we do this task though?

I don't think we've abandoned the idea, we just haven't been explicit about it. We can certainly talk about it some more.

As Thalia says, I don't think that blocks this task.

@dbarratt We should discuss that as a team. Does it impact on whether we do this task though?

Oh no it doesn't, I was just surprised. :) Apologies, didn't mean to distract. :)

Niharika set the point value for this task to 3.Feb 20 2020, 7:26 PM

Couple questions:

  • Define: full coverage
  • How useful is it really to test a special page? What exactly is it expected to be covered here?
  • Same as above for pagers. Specially when most of the methods are calling a method in the service.

The general idea of this specific task is to provide the full coverage for the Services, not so much for the special page, which seemed to be more focused and doable. We've added the use cases to guide/inform the approach to the testing.

Does that clarify the ticket better, @dmaza ?

Just to summarise a couple of discussions we had today - we'll update ComparePagerTest and CompareServiceTest along with T245499, to avoid the two tasks conflicting.

Change 576185 had a related patch set uploaded (by Dmaza; owner: Dmaza):
[mediawiki/extensions/CheckUser@master] Improve test coverage on CheckUser extension

Change 576185 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@master] Improve test coverage on CheckUser extension