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.
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.
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:
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.
- 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)
- Many uncovered things are related to global filters (we'll probably need a separated phpunit class for them)
- Several uncovered lines are due to cache stuff (for instance stashed filter results in checkAllFilters)
- Several uncovered lines are due to untested actions (for instance blockautopromote)
- 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)
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
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.
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.
@Huji thanks, I'll take a look. For the moment, just a little note: this seems to happen in any of the following cases:
- Actual code
- Using wfGetDB with $wgAbuseFilterCentralDB as third param
- Using loadBalancer (actual code), but outside of the cache callback
- 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.
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.