Page MenuHomePhabricator

Code coverage is low in AbuseFilter, Aim to increase to 50%
Closed, ResolvedPublic

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

Details

ProjectBranchLines +/-Subject
mediawiki/extensions/AbuseFiltermaster+294 -31
mediawiki/extensions/AbuseFiltermaster+347 -26
mediawiki/extensions/AbuseFiltermaster+102 -6
mediawiki/extensions/AbuseFiltermaster+167 -98
mediawiki/extensions/AbuseFiltermaster+189 -1
mediawiki/extensions/AbuseFiltermaster+298 -49
mediawiki/extensions/AbuseFiltermaster+69 -3
mediawiki/extensions/AbuseFiltermaster+580 -39
mediawiki/extensions/AbuseFiltermaster+8 -0
mediawiki/extensions/AbuseFiltermaster+189 -299
mediawiki/extensions/AbuseFiltermaster+130 -6
mediawiki/extensions/AbuseFiltermaster+245 -18
mediawiki/extensions/AbuseFiltermaster+220 -3
mediawiki/extensions/AbuseFiltermaster+370 -10
mediawiki/extensions/AbuseFiltermaster+51 -7
mediawiki/extensions/AbuseFiltermaster+12 -4
mediawiki/extensions/AbuseFiltermaster+35 -1
mediawiki/extensions/AbuseFiltermaster+260 -1
mediawiki/extensions/AbuseFiltermaster+7 -2
mediawiki/extensions/AbuseFiltermaster+48 -1
mediawiki/extensions/AbuseFiltermaster+146 -25
mediawiki/extensions/AbuseFiltermaster+30 -1
mediawiki/extensions/AbuseFiltermaster+29 -3
mediawiki/extensions/AbuseFiltermaster+20 -1
mediawiki/extensions/AbuseFiltermaster+55 -54
Show related patches Customize query in gerrit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

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

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

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

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:

  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

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

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

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

Change 568029 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] tests: Increase and rebalance code coverage

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

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 627510 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add tests for 'upload' action

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

Change 627766 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add test traits for uploads and account creation

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

Change 629865 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add a lot of selenium tests for the editing view

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

Change 627510 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add tests for 'upload' action

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

Change 629865 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add a lot of selenium tests for the editing view

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

Change 629865 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add a lot of selenium tests for the editing view

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

@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() )-

Change 629865 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add a lot of selenium tests for the editing view

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

@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).

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).

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/AbuseFilter/+/631311

Change 455871 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add tests for retrieving RC variables

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

Change 627766 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add test traits for uploads and account creation

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

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).

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/AbuseFilter/+/631311

This is still failing randomly, it seems https://integration.wikimedia.org/ci/job/wmf-quibble-selenium-php72-docker/63833/console It did pass on a recheck. Just letting you know :)

This is still failing randomly, it seems https://integration.wikimedia.org/ci/job/wmf-quibble-selenium-php72-docker/63833/console It did pass on a recheck. Just letting you know :)

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.)

I'm working on this, although the task is quite tracking/neverending. Guess we can call this resolved once coverage reaches 50%.

Peachey88 renamed this task from Code coverage is low in AbuseFilter to Code coverage is low in AbuseFilter, Aim to increase to 50%.Dec 19 2020, 12:26 PM

Change 650738 had a related patch set uploaded (by Matěj Suchánek; owner: Matěj Suchánek):
[mediawiki/extensions/AbuseFilter@master] Improve code coverage

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

Change 647270 had a related patch set uploaded (by Matěj Suchánek; owner: Matěj Suchánek):
[mediawiki/extensions/AbuseFilter@master] Test some Consequence classes and clean up

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

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

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

Change 647270 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Test some Consequence classes and clean up

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

Change 473456 abandoned by Daimona Eaytoy:
[mediawiki/extensions/AbuseFilter@master] Add new complete unit tests for most variables

Reason:

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