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

Simplify the _get_default_display_method tests using unittest.mock #3537

Merged
merged 7 commits into from
Oct 22, 2024

Conversation

seisman
Copy link
Member

@seisman seisman commented Oct 19, 2024

Refer to pytest-dev/pytest#4576 (comment) for comparisons of pytest.monkeypatch, unittest.mock and pytest-mock.

unittest.mock provides more features than pytest.monkeypatch which can simplify our tests. For example, with mock.call_count, we can directly count how many times a function/method is called, rather than implementing our own counter (like what're doing in

def _mock_ctypes_cdll_return(self, libname): # noqa: ARG002
).

Personally, I prefer pytest-mock which is as powerful as unittest.mock and can be used as a fixture, while unittest.mock must be used as a context manager or a decorator.

I think we have many options:

  1. pytest.monkeypatch only, which means in some cases, we have to write more codes for tests
  2. unittest.mock only or pytest-mock only
  3. pytest.monkeypatch + unittest.mock
  4. pytest.monkeypatch + pytest-mock

@seisman seisman added maintenance Boring but important stuff for the core devs skip-changelog Skip adding Pull Request to changelog needs review This PR has higher priority and needs review. labels Oct 19, 2024
@seisman seisman added this to the 0.14.0 milestone Oct 19, 2024
@seisman seisman force-pushed the tests/get_default_display_method branch from 22358cc to 541df3d Compare October 19, 2024 09:43
Comment on lines 436 to 438
# Mock IPython.get_ipython() to return an object with a config attribute, so PyGMT
# can detect that an IPython kernel is running.
@patch("IPython.get_ipython", return_value=Mock(config={"IPKernelApp": True}))
Copy link
Member

Choose a reason for hiding this comment

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

Haven't played around with unittest.mock much yet, but does this unittest.mock.path call need to be here on top as a decorator, rather than in the test function below? I find this looks a little unintuitive (even though it is less code) compared to setting up the MockIPython class inside the unit test which makes it clear what is happening.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here are different ways to mock IPython:

  1. Define a custom class, as in the current main branch and use pytest.monkeypatch
class MockIPython:
    def __init__(self):
        self.config = {"IPKernelApp": True}

    # Mock IPython.get_ipython() to return a MockIPython instance.
    mock_ipython = MockIPython()
    monkeypatch.setattr(IPython, "get_ipython", lambda: mock_ipython)
  1. Use mock/patch as a decorator
@patch("IPython.get_ipython", return_value=Mock(config={"IPKernelApp": True}))
def test_xxxx():
    ...
  1. Use mock/patch as a context manager
def test_xxxx():
    with patch("IPython.get_ipython", return_value=Mock(config={"IPKernelApp": True})):
        ....

Maybe we should use option 3 although it means more indentations?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, option 3 sounds good.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I've changed it to context manager in d08001c.

@seisman seisman changed the title Simplify the _get_default_display_method tests using mock Simplify the _get_default_display_method tests using unittest.mock Oct 21, 2024
@seisman seisman removed the needs review This PR has higher priority and needs review. label Oct 22, 2024
@seisman seisman merged commit b233e1c into main Oct 22, 2024
18 checks passed
@seisman seisman deleted the tests/get_default_display_method branch October 22, 2024 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs skip-changelog Skip adding Pull Request to changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants