Skip to content
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.

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

Refactor test_commandhandler.py #1408

Merged

Conversation

@plammens
Copy link
Contributor

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}
  - 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 label Sep 6, 2019
@tsnoam tsnoam self-requested a review Oct 11, 2019
@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
Copy link
Member

jh0ker left a comment

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
5 checks passed
5 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.