Skip to content

Refactor test_commandhandler.py #1408

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
tsnoam merged 1 commit into from
Oct 12, 2019
Merged

Refactor test_commandhandler.py #1408

tsnoam merged 1 commit into from
Oct 12, 2019

Conversation

@plammens
Copy link
Contributor

@plammens plammens commented May 19, 2019

Summary

  • Improved usage of fixtures
    • Replaced fixtures for directly callable factories where
      multiple mock objects were needed in the same test function
    • Extracted fixtures where possible (in place of literals or
      global constants)
  • Moved some fixtures to conftest.py to be used by other
    modules
  • Made a common base class for both TestCommandHandler and
    TestPrefixHandler, extracting common methods, patterns and
    signatures
    • The extracted patterns in test methods have been named with
      leading _test
  • Extracted other repeatedly used test utilities into functions
    (e.g. is_match) and methods (e.g. make_default_handler)

Rationale

The main goal was to improve maintainability. Here are some of the reasons for each of the changes.

Replacing fixtures for factory functions

Some test functions requested a fixture and used the same object multiple times, "patching" the object with the necessary changes to make each assertion, which was error-prone and cluttered the code. For example, this is how test_basic in TestCommandHandler was before:

    def test_basic(self, dp, message):
        handler = CommandHandler('test', self.callback_basic)
        dp.add_handler(handler)

        message.text = '/test'
        dp.process_update(Update(0, message))
        assert self.test_flag

        message.text = '/nottest'
        check = handler.check_update(Update(0, message))
        assert check is None or check is False

        message.text = 'test'
        check = handler.check_update(Update(0, message))
        assert check is None or check is False

        message.text = 'not /test at start'
        check = handler.check_update(Update(0, message))
        assert check is None or check is False

        message.entities = []
        message.text = '/test'
        check = handler.check_update(Update(0, message))
        assert check is None or check is False

The single message fixture is "reused" by patching the message.text field every time. That leads to a broken test, though, since the message.entities field isn't being changed appropriately: for example, if the third text was 'atest /test at start' instead of not /test at start, the corresponding assertion would fail (since the entity in message.entities still pointed at message.text[0]), and that's not the intended behaviour.

Now this test looks like this:

   def test_basic(self, dp, command):
        """Test whether a command handler responds to its command
        and not to others, or badly formatted commands"""
        handler = self.make_default_handler()
        dp.add_handler(handler)

        assert self.response(dp, make_command_update(command))
        assert not is_match(handler, make_command_update(command[1:]))
        assert not is_match(handler, make_command_update('/not{}'.format(command[1:])))
        assert not is_match(handler, make_command_update('not {} at start'.format(command)))

Where command is a fixture for a command, like '/test', and make_command_updateis a factory that handles making an accurate telegram.Message with the appropiate entity.

Base class for both test classes

The two test classes, TestCommandHandler and TestPrefixHandler, were very very similar, so a base class for both with common fixtures, callbacks and other utility methods helped a lot in cleaning the code

Fixtures in conftest.py

Some fixtures were moved to conftest.py so that they can be used by other test modules. Grouping test modules into subdirectories with their own conftest.py with shared fixtures and utilities would be ideal, but I only worked on test_commandhandler.py.

Other details

Tests for pass_*

I had a hard time understanding why pass_user_data, pass_chat_data, pass_job_queue, and pass_update_queue were tested in pairs:

    def test_pass_user_or_chat_data(self, dp, message):
        handler = CommandHandler('test', self.callback_data_1,
                                 pass_user_data=True)
        dp.add_handler(handler)

        message.text = '/test'
        dp.process_update(Update(0, message=message))
        assert self.test_flag

        dp.remove_handler(handler)
        handler = CommandHandler('test', self.callback_data_1,
                                 pass_chat_data=True)
        dp.add_handler(handler)

        self.test_flag = False
        dp.process_update(Update(0, message=message))
        assert self.test_flag

        dp.remove_handler(handler)
        handler = CommandHandler('test', self.callback_data_2,
                                 pass_chat_data=True,
                                 pass_user_data=True)
        dp.add_handler(handler)

        self.test_flag = False
        dp.process_update(Update(0, message=message))
        assert self.test_flag

    def test_pass_job_or_update_queue(self, dp, message):
        handler = CommandHandler('test', self.callback_queue_1,
                                 pass_job_queue=True)
        dp.add_handler(handler)

        message.text = '/test'
        dp.process_update(Update(0, message=message))
        assert self.test_flag

        dp.remove_handler(handler)
        handler = CommandHandler('test', self.callback_queue_1,
                                 pass_update_queue=True)
        dp.add_handler(handler)

        self.test_flag = False
        dp.process_update(Update(0, message=message))
        assert self.test_flag

        dp.remove_handler(handler)
        handler = CommandHandler('test', self.callback_queue_2,
                                 pass_job_queue=True,
                                 pass_update_queue=True)
        dp.add_handler(handler)

        self.test_flag = False
        dp.process_update(Update(0, message=message))
        assert self.test_flag

In any case, due to their similarity, I condensed together in a single parametrised test method:

@pytest.mark.parametrize('pass_keyword', BaseTest.PASS_KEYWORDS)
    def test_pass_data(self, dp, command_update, pass_combination, pass_keyword):
        handler = CommandHandler('test', self.make_callback_for(pass_keyword), **pass_combination)
        dp.add_handler(handler)
        assert self.response(dp, command_update) == pass_combination.get(pass_keyword, False)

I kept the "testing in pairs" by parametrising the pass_combination fixture appropriately:

PASS_KEYWORDS = ('pass_user_data', 'pass_chat_data', 'pass_job_queue', 'pass_update_queue')

@pytest.fixture(scope='module', params=itertools.combinations(PASS_KEYWORDS, 2))
def pass_combination(self, request):
    return {key: True for key in request.param}

@plammens
tests: refactor test_commandhandler.py
f055d80 
  - Improved usage of fixtures
    - Replaced fixtures for directly callable factories where
    multiple mock objects were needed in the same test function
    - Extracted fixtures where possible (in place of literals or
    global constants)
  - Moved some fixtures to ``conftest.py`` to be used by other
  modules
  - Made a common base class for both ``TestCommandHandler`` and
  ``TestPrefixHandler``, extracting common methods, patterns and
  signatures
    - The extracted patterns in test methods have been named with
    leading ``_test``
  - Extracted other repeatedly used test utilities into functions
  (e.g. ``is_match``) and methods (e.g. ``make_default_handler``)
@Bibo-Joshi Bibo-Joshi added the ⚙️ tests affected functionality: tests label Sep 6, 2019
@tsnoam tsnoam self-requested a review October 11, 2019 20:11
@tsnoam tsnoam self-assigned this Oct 11, 2019
@tsnoam tsnoam added this to the 12.2 milestone Oct 11, 2019
jh0ker
jh0ker approved these changes Oct 11, 2019
View reviewed changes
Copy link
Member

@jh0ker jh0ker left a comment

Choose a reason for hiding this comment

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

Very cool PR, sorry that there hasn't been any movement on it for such a long time. IMO this is good to merge.

@tsnoam tsnoam merged commit 3318239 into python-telegram-bot:master Oct 12, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Aug 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jh0ker jh0ker jh0ker approved these changes

@tsnoam tsnoam Awaiting requested review from tsnoam

Assignees

@tsnoam tsnoam

Labels

⚙️ tests affected functionality: tests

Projects

None yet

Milestone

12.3

Development

Successfully merging this pull request may close these issues.

4 participants

@plammens @jh0ker @tsnoam @Bibo-Joshi