Page MenuHomePhabricator

Code coverage is low in AbuseFilter
Open, HighPublic

Description

At the time of this writing, https://doc.wikimedia.org/cover-extensions/AbuseFilter/ shows about 20% coverage (by lines or by functions) for what is in ./includes and 0% for what is in ./maintenance. Coverage by classes and traits is 2% for ./includes and 0% for ./maintenance.

Progress: https://tools.wmflabs.org/coverage/charts/AbuseFilter.png

Event Timeline

Huji created this task.Aug 3 2018, 6:18 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 3 2018, 6:18 PM
Huji triaged this task as High priority.Aug 3 2018, 6:19 PM

The solution involves adding more regression tests (as is being done in T42478), as well as other tests not focused on regressions (e.g. tests focused on the maintenance scripts).

He7d3r added a subscriber: He7d3r.Aug 3 2018, 6:23 PM
Restricted Application added a subscriber: Daimona. · View Herald TranscriptAug 3 2018, 7:17 PM

Agreed. We should probably devise some steps to follow for doing this. My suggestion is to focus first on "internal" stuff, the defects of which may be sneakier and harder to notice. This means to increase coverage for the includes/parser folder, which is already the most covered one, and a couple of other files like VariableHolder. Then I guess we should think about the AbuseFilter class, which will also be covered by the remaining patch of the subtask; the class is used roughly everywhere, and such importance requires reliable code. Then the rest of the code, being Views, special pages, api stuff and so on (all inside the includes folder). I think maintenance has the lowest priority. Of course this might not be the best solution, nor I'll be able to actively work on it until the end of the month, so I'd like to hear other thoughts and I'd also be glad to help as much as I can.

Legoktm added a subscriber: Legoktm.Aug 3 2018, 7:40 PM

I'd also suggest taking a look at https://tools.wmflabs.org/coverme/?repo=Extension%3AAbuseFilter&type=all - it sorts all the functions by how often they're called in Wikimedia production, so you can prioritize covering the most often called code. (of course, being called the most doesn't necessarily mean it should be the priority of what needs coverage, it's possible that that code is a lot more stable and battle tested than code that is called rarely).

I'm currently writing a patch for code coverage in AbuseFilterParser. While most things can be covered more, there are some troubles in CoverMe and wikimedia docs:

  1. Line 873 is in red although the exception is tested (everything else is green)
  2. Some functions are reported to have less than 100% coverage, but they actually are fully covered (for instance, see here funcLc and compare with this)

Change 453996 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Improve code coverage for AbuseFilterParser

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

Daimona added a comment.EditedAug 20 2018, 11:27 AM

Oh, and also: for testing we use normal parser and CachingParser. While we should make the latter work and eventually use it everywhere, we can't make it "voting" in tests, as it prevents to add tests which would work fine with the normal parser.

Another note: I looked at code coverage in AbuseFilter class, and this is what I understood from it.

  1. Some methods show up as not covered although they are actually executed in at least one test (for instance, AbuseFilter::doSaveFilter); also, some lines are red although such code should be executed (see example above from line 873)
  2. Many uncovered things are related to global filters (we'll probably need a separated phpunit class for them)
  3. Several uncovered lines are due to cache stuff (for instance stashed filter results in checkAllFilters)
  4. Several uncovered lines are due to untested actions (for instance blockautopromote)
  5. Some points in the code may be covered by playing with condition limit (try to execute a filter when the limit has been hit) or wgAbuseFilterSlowFilterRuntimeLimit (again, making a filter hit the time limit)
Huji added a comment.Aug 20 2018, 1:21 PM
  1. Some functions are reported to have less than 100% coverage, but they actually are fully covered (for instance, see here funcLc and compare with this)

@Legoktm can you please take a look and see if you can explain this?

Daimona added a comment.EditedAug 20 2018, 2:02 PM

As another example of this, you may check for instance lines 610-611 or lines 968, 970-71

Another problem: the AFPData class is reported to have almost no coverage, but its methods are actually called in parserTests. Why's that?

Change 454027 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Improve tests for the AbuseFilter class

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

Change 453996 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Improve code coverage for AbuseFilterParser

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

Change 454518 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add missing @covers tag

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

Change 454518 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add missing @covers tag

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

Legoktm updated the task description. (Show Details)Aug 22 2018, 3:41 PM

Some of these potentially look like PHPUnit/Xdebug bugs that we'll need to work into smaller reproduction cases. I think you also figured out that some @covers tags were missing.

Also for cases like https://doc.wikimedia.org/cover-extensions/AbuseFilter/includes/parser/AFPData.php.html#307 where it should never happen, and therefore is impossible to write a test for, feel free to use // @codeCoverageIgnoreLine right before it

Change 454589 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Improve code coverage

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

Daimona added a comment.EditedAug 23 2018, 8:27 AM

Yes, now @covers tag should be right and in fact the coverage increased. As for untestable code, I'm not sure if it's worth it adding annotations everywhere: we could just be fine with 99% coverage or so, I guess.
Also, what should we use to check HTML? We'd need something to make sure that the output from View classes is correct, and maybe we should also check request values etc.

Change 454589 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Improve code coverage

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

Change 454782 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Improve parser coverage

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

Change 454782 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Improve parser coverage

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

Change 454027 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Improve tests for the AbuseFilter class

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

Change 454888 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] [WIP] Add unit tests for stashed edits

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

Change 455173 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add unit tests for profiling

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

Change 455291 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Improve coverage for AbuseFilterTokenizer

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

Change 455324 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] [WIP] Add tests for global filters

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

Daimona added a comment.EditedAug 25 2018, 3:09 PM

I'm facing a problem when testing global filters (patch right above). For the test, we would ideally need two databases, one for local filter and one for global ones. Since I'm not aware of a way to create another database and then use both in tests, I just saved global filters to the existent DB. However, when in the AF code we search the external DB for global filters (here), the DB prefix 'unittest_' is gone, and thus the test fails. So my question is: is there a way to have two working DBs in a test or, if no, why is the prefix gone? CC'ing @Legoktm.

Daimona added a comment.EditedAug 25 2018, 3:40 PM

@Huji thanks, I'll take a look. For the moment, just a little note: this seems to happen in any of the following cases:

  1. Actual code
  2. Using wfGetDB with $wgAbuseFilterCentralDB as third param
  3. Using loadBalancer (actual code), but outside of the cache callback
  4. Using wfGetDB with $wgAbuseFilterCentralDB as third param outside of the cache callback

Getting local filters works perfectly fine instead.

EDIT: Unfortunately, CA tests don't seem to help. Also, I can't figure out if this is some sort of bug in MediaWikITestCase class or if we're using it the wrong way.

Change 455871 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add tests for retrieving RC variables

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

Change 456089 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add full tests for deprecated variables

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

Change 454557 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Restore (temporarily?) unit tests for CachingParser and fix it

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

Daimona claimed this task.Aug 29 2018, 4:13 PM

Slowly going up, this will need some time (and many other patches) anyway.

Change 456089 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add full tests for deprecated variables

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

Change 455291 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Improve coverage for AbuseFilterTokenizer

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

Change 473456 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add new complete unit tests for most variables

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

Change 481524 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add tests for storing and loading the variables dump

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

Change 481524 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add tests for storing and loading the variables dump

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

Change 454888 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add unit tests for stashed edits

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

Change 455173 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add unit tests for profiling

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

Change 455324 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add tests for global filters

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

Change 454557 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Restore (temporarily?) unit tests for CachingParser and fix it

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

Change 481524 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add tests for storing and loading the variables dump

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

Change 503654 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Increase code coverage a bit

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

Change 503654 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Increase code coverage a bit

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

Change 503658 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Tweak coverage part 2

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

Change 503658 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Tweak coverage part 2

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

Change 503724 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add tests for tokenizer caching

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

Change 503738 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add tests for various data type casts

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

Change 503724 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add tests for tokenizer caching

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

@Daimona I'd like to help, but know little-to-nothing about code coverage - how does one learn / what can I do?

@DannyS712 Thanks for the help! There are some good explanations on the web, just searching "code coverage" on google yields something interesting. Basically, code coverage is the amount of code which gets executed during tests.

How are tests written / created?

@DannyS712: Please take a look at previous patches linked in this task.

Change 454888 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add unit tests for stashed edits

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

Change 455173 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add unit tests for profiling

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

Change 455324 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add tests for global filters

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

Change 503738 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add tests for various data type casts

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

Change 454557 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Restore unit tests for CachingParser and fix it

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

FTR, given that there are lots of patches bound to this task, the only missing ones are https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/455871/ and https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/473456/; It'll take some time to update the second.

Change 512476 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add missing covers/group tags

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

Change 512476 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add missing covers/group tags

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