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.
|Resolved||Daimona||T201193 Code coverage is low in AbuseFilter, Aim to increase to 50%|
|Resolved||Daimona||T42478 AbuseFilter needs regression tests|
|Resolved||Daimona||T193596 Abstract out parts of AbuseFilterViewEdit that deal with saving a filter|
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.
@Daimona one of these tests is failing at https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MassMessage/+/631144 which shouldn't be related?
✖ cannot save an empty filter
The expression evaluated to a falsy value: assert( ViewEditPage.error.isDisplayed() )-
Hmm, it's not the first time I see it, but it always passes locally. Given what the test does, my guess is, it tries clicking the "submit" button before it works. This is not 100% convincing because the button is not JS-enriched etc., but perhaps it's worth trying. As anticipated, I cannot tell whether the fix will work because I don't get a failure locally (not even if I delay the page load).
Hm :-/ I can't understand why. Given what the test does, I'd expect that the page might not be ready when we try clicking the button. OTOH, we're using waitForClickable(), plus that's a simple submit button with no special JS logic, so it should work immediately. Based on the comments at T264926, can we exclude that the test framework is bugged?
That said, if this failure happens again and blocks stuff, please do feel free to disable the test (by replacing it( with it.skip().
Removing task assignee due to inactivity, as this open task has been assigned to the same person for more than two years (see the emails sent to the task assignee on Oct27 and Nov23). Please assign this task to yourself again if you still realistically [plan to] work on this task - it would be welcome.
(See https://www.mediawiki.org/wiki/Bug_management/Assignee_cleanup for tips how to best manage your individual work in Phabricator.)