Page MenuHomePhabricator
Create Task
Maniphest T174195

More extensive unit testing for AntiSpoof
Closed, ResolvedPublic3 Estimated Story Points
Actions

Description

Right now there is only very limited testing of AntiSpoof data. We should make this much more extensive and comprehensive.

Acceptance criteria

  • Propose to AHT and CommTech what the unit tests should cover
  • Incorporate any feedback from other devs
  • Implement the tests

Tests

  • Testing whatever function(s) are in the library for doing string comparison
  • Making sure we test at least one equivalency that involves recursive mapping, e.g. Θ -> 0 -> O
  • Making sure we test at least one equivalency that involves case change
  • Testing a few random spoof strings, e.g. j1mmy w4l35 (no idea if that one even works)
  • Sequence processing like vv -> w and rn -> m that we should not forget to include
  • All the different badCharErr fail situations (?)

Details

SubjectRepoBranchLines +/-
Add more unit tests.mediawiki/extensions/AntiSpoofmaster+30 -2
Customize query in gerrit

Related Objects
Search...

Event Timeline

kaldari created this task.Aug 25 2017, 7:04 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 25 2017, 7:04 PM
• TBolliger added a parent task: T166816: Epic ⚡️ : Accuracy improvements to anti-spoof tools across multiple pertinent tools.Aug 25 2017, 7:05 PM
• TBolliger moved this task from Untriaged to Triage/To be Estimated on the Anti-Harassment board.Aug 29 2017, 4:49 PM
• TBolliger updated the task description. (Show Details)Sep 6 2017, 10:13 PM
• TBolliger set the point value for this task to 3.Sep 8 2017, 7:23 PM
• TBolliger moved this task from Triage/To be Estimated to Cards ready for development on the Anti-Harassment board.
dmaza moved this task from Cards ready for development to AHT Sprint 5 on the Anti-Harassment board.Sep 12 2017, 6:45 PM
dmaza edited projects, added Anti-Harassment (AHT Sprint 5); removed Anti-Harassment.
dbarratt claimed this task.Sep 26 2017, 8:57 PM
dbarratt moved this task from Ready to In progress on the Anti-Harassment (AHT Sprint 5) board.
• TBolliger moved this task from AHT Sprint 5 to AHT Sprint 6 on the Anti-Harassment board.Sep 27 2017, 6:24 PM
• TBolliger edited projects, added Anti-Harassment (AHT Sprint 6); removed Anti-Harassment (AHT Sprint 5).
dbarratt moved this task from Ready to In progress on the Anti-Harassment (AHT Sprint 6) board.Sep 27 2017, 6:36 PM
dbarratt added a subscriber: Huji.Sep 28 2017, 1:48 PM

@kaldari Here are my thoughts:

  • We should probably add tests cases to the new library we create in T174197, which is what @Huji asked for anyways.
  • We could use the AntiSpoof logs to get examples of when the filter was tripped. This would create regression tests for us. I'm not sure how helpful that would be because I can't see us removing characters, but I guess it protects against regressions.
  • As far as getting examples of where AntiSpoof was not tripped, but it should have. I'm not sure how we'd do this other than soliciting community feedback? I suppose we could query for all the usernames, but I'm not sure how we'd determine which ones should have been flagged. I guess we could run them all through Spoofchecker::isSuspicious and that would at least give us a list to take a look at.

What do you think?

dbarratt reassigned this task from dbarratt to kaldari.Oct 4 2017, 12:38 PM
dbarratt subscribed.
kaldari reassigned this task from kaldari to dbarratt.Oct 6 2017, 8:22 PM

@dbarratt: I don't think we need to go crazy with testing the equivalencies. Basically, I just want to make sure we have the following covered:

  • Testing whatever function(s) are in the library for doing string comparison
  • Making sure we test at least one equivalency that involves recursive mapping, e.g. Θ -> 0 -> O
  • Making sure we test at least one equivalency that involves case change
  • Testing a few random spoof strings, e.g. j1mmy w4l35 (no idea if that one even works)
dmaza subscribed.Oct 8 2017, 6:05 AM

Other things to test for:

  1. Sequence processing like vv -> w and rn -> m that we should not forget to include
  2. All the different badCharErr fail situations (?)
dbarratt edited projects, added Anti-Harassment (AHT Sprint 7); removed Anti-Harassment (AHT Sprint 6).Oct 11 2017, 6:33 PM
dbarratt moved this task from Ready to In progress on the Anti-Harassment (AHT Sprint 7) board.
dbarratt removed dbarratt as the assignee of this task.Oct 11 2017, 8:53 PM
dbarratt moved this task from In progress to Ready on the Anti-Harassment (AHT Sprint 7) board.
dbarratt moved this task from Ready to In progress on the Anti-Harassment (AHT Sprint 7) board.Oct 19 2017, 2:57 PM
dbarratt claimed this task.Oct 19 2017, 11:21 PM
dbarratt updated the task description. (Show Details)
dbarratt added a comment.Oct 19 2017, 11:48 PM
In T174195#3666061, @kaldari wrote:
  • Making sure we test at least one equivalency that involves recursive mapping, e.g. Θ -> 0 -> O

So I'm adding a test for this case, but it doesn't actually look like we do recursive mapping, we actually have both variations in the mapping:
https://github.com/wikimedia/Equivset/blob/5dcb3d77a6eaf9cb65ed45b547ddc839c246a49c/data/equivset.in#L311
and
https://github.com/wikimedia/Equivset/blob/5dcb3d77a6eaf9cb65ed45b547ddc839c246a49c/data/equivset.in#L329

dbarratt updated the task description. (Show Details)Oct 19 2017, 11:48 PM
dbarratt updated the task description. (Show Details)Oct 20 2017, 3:44 PM
dbarratt updated the task description. (Show Details)
gerritbot subscribed.Oct 20 2017, 3:56 PM

Change 385392 had a related patch set uploaded (by Dbarratt; owner: Dbarratt):
[mediawiki/extensions/AntiSpoof@master] Add more unit tests.

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

gerritbot added a project: Patch-For-Review.Oct 20 2017, 3:56 PM
dbarratt added a comment.Oct 20 2017, 4:28 PM
In T174195#3667640, @dmaza wrote:

Sequence processing like vv -> w and rn -> m that we should not forget to include

This is a little weird... we are doing the sequence processing in AntiSpoof (not the equivset library)
https://phabricator.wikimedia.org/diffusion/EANS/browse/master/AntiSpoof_body.php;31ff54dfbedcf49b8cde9ec8bd53f4f79a5f194e$382

It looks like tests already exist for that (since I had to remove them for equivset), and it seems a little half baked. I wonder if we should add sequence processing to equivset? or should that be a new library?

It looks like AbuseFilter was never doing sequence processing since it's outside of the normalizeString method.

Regardless, I will leave the tests in AntiSpoof for sequence processing.

dmaza added a comment.Oct 20 2017, 4:42 PM

Considering that Equivset is just a mapping of "similar" looking characters I think sequence processing should not be part of it.

dbarratt updated the task description. (Show Details)Oct 20 2017, 5:17 PM
In T174195#3667640, @dmaza wrote:

All the different badCharErr fail situations (?)

It took me a while to find a bad character that would trip the error, but I found one and included it in T177667

dbarratt updated the task description. (Show Details)Oct 20 2017, 5:19 PM
dbarratt moved this task from In progress to Code Review on the Anti-Harassment (AHT Sprint 7) board.
dbarratt added a subtask: T174197: Split off AntiSpoof equivset generation and string normalization into its own library.Oct 20 2017, 11:42 PM
dbarratt added a subtask: T177667: Get Equivset Test Coverage to 100%.
dbarratt added a project: Equivset.Oct 23 2017, 2:45 PM
dbarratt closed subtask T177667: Get Equivset Test Coverage to 100% as Resolved.Oct 23 2017, 9:56 PM
dbarratt moved this task from Backlog to In Progress on the Equivset board.Oct 25 2017, 3:25 PM
• TBolliger moved this task from AHT Sprint 7 to AHT Sprint 8 on the Anti-Harassment board.Oct 25 2017, 6:39 PM
• TBolliger edited projects, added Anti-Harassment (AHT Sprint 8); removed Anti-Harassment (AHT Sprint 7).
• TBolliger moved this task from Ready to Code Review on the Anti-Harassment (AHT Sprint 8) board.Oct 25 2017, 6:42 PM
dbarratt closed this task as Resolved.Nov 6 2017, 7:28 PM
dbarratt moved this task from Code Review to Done on the Anti-Harassment (AHT Sprint 8) board.
gerritbot added a comment.Nov 6 2017, 7:32 PM

Change 385392 merged by jenkins-bot:
[mediawiki/extensions/AntiSpoof@master] Add more unit tests.

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

ReleaseTaggerBot added a project: MW-1.31-release-notes (WMF-deploy-2017-11-07 (1.31.0-wmf.7)).Nov 6 2017, 8:01 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL