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

Support for type checking any mock with spec #121

Open
fornellas opened this issue Apr 20, 2020 · 10 comments
Open

Support for type checking any mock with spec #121

fornellas opened this issue Apr 20, 2020 · 10 comments

Comments

@fornellas
Copy link

While working on TestSlide (a Python test framework from Facebook), @david-caro, @fabriziocucci and myself found an issue with typeguard integration.

In summary, typeguard checks for types recursively (eg: for Union, Tuple etc), which is a good thing. However, we got failures when one of these recursive checks hits a mock object.

TestSlide type validation functionality plays nicely with mocks, and can detect and extract their templates for use in type validation. But, if a mock has no template, no type validation happens (just like typeguard).

TestSlide's interface for this is public and extensible, so we can add supports for any mock object, including TestSlide's own StrictMock.

I cut this clowny PR for TestSlide, to add the extra logic to deal with mocks, by patching typeguard.check_type and wrapping it, so we can move on temporarily.

@agronholm , I'm willing to cut a PR for typeguard to implement this public interface for dealing with mocks, what do you think? Once available at typeguard, we can drop it from TestSlide.

@agronholm
Copy link
Owner

I've recently merged a PR for bypassing type checks for unittest.[Magic]Mock objects. I just haven't cut a release since then. Would it suffice for you if I were to make a new release now?

@fornellas
Copy link
Author

That's in a good direction, but still does not cover these 2 cases:

  • Using the mock spec for type checking when available.
  • Have a public API to extend support for any third party mocks.

What do you think?

@agronholm
Copy link
Owner

For the first item, I would be inclined to accept a PR for that. For the second item, I would consider a proposal for such an API.

@fornellas
Copy link
Author

Cool! I'll do a PR for the first item as soon as I have time.

For the second, here's the proposal:

https://github.com/facebookincubator/TestSlide/blob/0b36ffdb40d471d9e945864647265f0033f2d521/testslide/lib.py#L18-L42

MOCK_TEMPLATE_EXTRACTORS would be public, and people can extend it, to add their custom mock template extractor functions like this:

https://github.com/facebookincubator/TestSlide/blob/0b36ffdb40d471d9e945864647265f0033f2d521/testslide/strict_mock.py#L734-L741

@agronholm
Copy link
Owner

This feature is trickier to implement than I thought at first because it requires a class compatibility check instead of the instance/value checks that typeguard currently uses.

@wanderrful
Copy link

wanderrful commented Aug 6, 2020

+1 Would like to see this happen. Mocks of third party package classes are being flagged by Typeguard as being incorrect types because of the way I'm setting the Mocks' return_value. In order to get around this, I'd have to create new instances of those third party packages, which defeats the purpose of Mocks and unit testing.

edit-- Typeguard docs say they ignore Mocks, but that doesn't seem to be the case: https://typeguard.readthedocs.io/en/latest/userguide.html?highlight=mock#support-for-mock-objects

edit^2 -- Actually it looks like I've unfortunately hit some kind of strange edge case scenario (explained below) where a mock is not used by Typeguard.

@agronholm
Copy link
Owner

Can you create a minimal script to demonstrate the problem?

@wanderrful
Copy link

wanderrful commented Aug 6, 2020

Unfortunately for me it seems to be a really weird edge case involving type expectations, because my attempts to reproduce it in a minimal way aren't working. I imagine the underlying reason could be because of the type expectations being more permissive when it's imported in a certain way or when the underlying classes are less complex.

class ThirdPartyPackageClass:
    """I should be able to mock this for unit tests without Typeguard complaining."""
    _internal_hash_map = dict()

    def __getitem__(self, item):
        return self._internal_hash_map[item]


class ClassIWantToTest:
    """This is the thing I wrote."""
    _foo: List[str] = list()

    def __init__(self, one: ThirdPartyPackageClass, key: str):
        if key not in one:
            raise Exception("Oops")
        self._foo = one[key]


def test_blah() -> None:
    """Typeguard is okay with this minimal example."""
    mock_data = {"hello": ["foo", "bar", "baz"]}
    my_mock = Mock(spec=ThirdPartyPackageClass, return_value=mock_data)

    a = ClassIWantToTest(my_mock(), "hello")  # noqa: F841
    assert a._foo == mock_data["hello"]

The idea is that we want to mock the minimal behavior for a unit test without needing to instantiate the entire class itself, which could potentially take up a lot of memory for integration/acceptance tests.

The underlying third party package in question here is Workbook from the openpyxl project: https://openpyxl.readthedocs.io/en/stable/api/openpyxl.workbook.workbook.html?highlight=workbook#openpyxl.workbook.workbook.Workbook

I imagine there must be something strange about its implementation that doesn't allow me to mock it naively for Typeguard (though Pytest and Mypy have no issue with it), and it somehow falls through the cracks with Typeguard.

In reality, what we get from Typeguard is something like this:

TypeError: type of argument "one" must be openpyxl.workbook.workbook.Workbook; got dict instead

What I'd expect from Typeguard is to recognize that I'm passing in a Mock object to the constructor of ClassIWantToTest and then just ignore it. I'm obviously just misunderstanding the documentation and intended behavior.

@agronholm
Copy link
Owner

The error you pasted here indicates that mocks are not used at all in the function call. Please paste the relevant target function header and the exact type of the object being passed.

@agronholm agronholm added this to the 3.0.0 milestone Oct 10, 2021
@agronholm agronholm removed this from the 3.0.0 milestone Dec 5, 2021
@agronholm
Copy link
Owner

I only have an xfailing test for this in the code base, but I don't think it's going to start working any time soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants