Skip to content

Antispam rules: add unit test for mentions, improve unit tests for links and attachments, fix bug in attachments rule#655

Merged
MarkKoz merged 18 commits into
python-discord:masterfrom
kwzrd:unittest-mentions
Dec 23, 2019
Merged

Antispam rules: add unit test for mentions, improve unit tests for links and attachments, fix bug in attachments rule#655
MarkKoz merged 18 commits into
python-discord:masterfrom
kwzrd:unittest-mentions

Conversation

@kwzrd
Copy link
Copy Markdown
Contributor

@kwzrd kwzrd commented Nov 9, 2019

This PR addresses several problems.

  • Implement unit test for bot.rules.mentions, using the MockMessage helper to correctly represent mocked discord.Message instances. Closes Write unit tests for bot/rules/mentions.py #601. Gives 100% coverage. See
    0eade46.
  • Adjust unit tests for bot.rules.links and bot.rules.attachments to use MockMessage helper instead of a namedtuple("FakeMessage", ["author", "content"]) that was a nasty oversimplification of actual Message instances. After Enhancements for tests.helpers and our test suite #660 the mocking system is now more safe as it prevents setting attributes that do not exist for the mocked objects. See 2a9b1fc and a2617d1.
  • Small docstring adjustment - use backticks instead of asterisks when referring to args. See 32caead.

Additionally, we have discovered that there is an inconsistency / bug in the behaviour of bot.rules.attachments. Snippets below explain.

All rules have the same function signature, taking the following args:

last_message: Message, recent_messages: List[Message], config: Dict[str, int]

The antispam cog calls them in on_message by providing the message that triggers the event, and a history of messages up to a certain point in time. This history, however, includes the message instance itself. So let's say last_message == recent_messages[0]. Within the rule, the first thing we do is build a tuple relevant_messages which is made up of all messages m where m.author == last_message.author, and so this is the only way the last_message arg is used - to see which user we're checking (slight deviation in burst rule which applies to all users):

relevant_messages = tuple(
    msg
    for msg in recent_messages
    if msg.author == last_message.author
)

The attachments rule however deviates from this pattern by always including the last_message in relevant_messages, and then looping over recent_messages. The result of this is that the last_message potentially gets included twice, and its attachments then count twice in the sum.

relevant_messages = [last_message] + [
    msg
    for msg in recent_messages
    if (
        msg.author == last_message.author
        and len(msg.attachments) > 0
    )
]

Therefore, a test case that has last_message == recent_messages[0] will fail, for example

last_message = msg(4)
recent_messages = [msg(4), msg(0), msg(6)]
expected_total = 10

... fails with ...

- ('sent 14 attachments in 5s',
?         ^

+ ('sent 10 attachments in 5s',
?         ^

... and also the relevant_messages containing a duplicate.

So why wasn't the rule failing on the unit test written for it?

The unit test incorrectly makes the assumption here and here that last_message is not present in recent_messages, therefore making the same mistake as the rule itself.

This PR therefore also contains the following.

  • Fix the attachments rule to understand last_message to be part of recent_messages and build relevant_messages accordingly. See 54598dd.
  • Adjust the unit test to emulate the behaviour of the on_message event calling the rule. See eef447a.
  • Add slightly more complex test cases. Test the attachments rule against multiple authors, not just a hardcoded lemon. See 01731a8.

Always happy to hear any feedback.

@kwzrd kwzrd added the a: tests Related to tests (e.g. unit tests) label Nov 9, 2019
Comment thread tests/bot/rules/test_mentions.py Outdated
@kwzrd kwzrd changed the title Add unit test for mentions antispam rule Antispam rules: add unit test for mentions, improve links and attachments unit tests, fix bug in attachments rule Nov 14, 2019
@kwzrd kwzrd changed the title Antispam rules: add unit test for mentions, improve links and attachments unit tests, fix bug in attachments rule Antispam rules: add unit test for mentions, improve unit tests for links and attachments, fix bug in attachments rule Nov 14, 2019
Copy link
Copy Markdown
Contributor

@MarkKoz MarkKoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_disallows_messages_with_too_many_attachments should be now acting like a regression test against that duplicate message bug for attachments, right?

Comment thread tests/bot/rules/test_attachments.py
@MarkKoz MarkKoz added s: waiting for author Waiting for author to address a review or respond to a comment and removed status: needs review labels Dec 3, 2019
@MarkKoz MarkKoz removed the s: waiting for author Waiting for author to address a review or respond to a comment label Dec 4, 2019
@MarkKoz MarkKoz merged commit 2475784 into python-discord:master Dec 23, 2019
@kwzrd kwzrd deleted the unittest-mentions branch December 24, 2019 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: tests Related to tests (e.g. unit tests) p: 1 - high High Priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Write unit tests for bot/rules/mentions.py

6 participants