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 graphing mocked methods #26

Merged
merged 1 commit into from Apr 13, 2016

Conversation

Projects
None yet
2 participants
@ashanbrown
Copy link
Contributor

ashanbrown commented Feb 23, 2016

I've found a problem using objgraph to graph the relationships involving methods mocked by the mock library. Specifically, when applied to a mocked instance method, _safe_repr returns a mock, rather than a string, which causes _obj_label to fail. This is because the mock library delegates __class__ to the spec it is using and it doesn't return mock.Mock (or some variant of that). This PR uses type() instead of __class__ to determine the class of an object.

Assuming you're open to this PR, I'd like to include tests with this PR, but I need to know whether whether you'd be open to include the mock library as a dependency (it is a standard library in python 3 but not before). If not, I can write lower level tests, but they wouldn't automatically break if mock ever changes how it is implemented (which may or may not be desirable). Thanks for your consideration and maintaining such a useful tool.

@mgedmin

This comment has been minimized.

Copy link
Owner

mgedmin commented Feb 24, 2016

Thank you for the PR!

I'm likely to be a bit busy in the next couple of weeks, so I cannot promise a prompt review. I'd like to understand what kind of error using mocks causes in obj_label -- I'm sure I'll be able to figure that out after running your new unit test against an unpatched objgraph.py, when I find the time to do so.

At first sight this looks like a change I'll want to include (after figuring out the CI failures). I'm not against adding a mock dependency for the test suite -- about all it'll do is make it impossible for me to test on older versions of Python, but I don't really want support 2.6 or 3.0–3.2 any more. I'll just have to make sure to bump the version number appropriately and mention that in the changelog, if I haven't already.

@ashanbrown

This comment has been minimized.

Copy link
Contributor

ashanbrown commented Feb 25, 2016

No rush on the PR. Thanks for your consideration of this. I've added a few more test cases and some more handling for unusual mock related cases. The tests should pass on all python versions. The first test wasn't a problem on python 3.x so I've only employed the issubclass substitution for isinstance for python 2.x, but the remaining test demonstrates problems on python 3.x as well.

I wasn't able to come up with an exact replica of what I was seeing with my app, which was that objgraph.show_backrefs was triggering an exception, rather than just saying (unrepresentable). That exception was caused by _short_repr returning a mock. In my tests, I'm able to simulate that effect just by passing a mock to __name__. My suggested changes try to ensure that whatever comes out of _short_repr is a string.

@mgedmin

This comment has been minimized.

Copy link
Owner

mgedmin commented Mar 7, 2016

Appveyor tests failed because mock isn't installed. That should be easy to fix.

@ashanbrown

This comment has been minimized.

Copy link
Contributor

ashanbrown commented Mar 31, 2016

@mgedmin Hi, sorry to disappear. I don't really have a good idea about why this isn't working on appveyor and I'm not eager to set up some sort of windows vm on my mac. If you have any suggestions, I'd welcome them. Thanks.

@mgedmin

This comment has been minimized.

Copy link
Owner

mgedmin commented Mar 31, 2016

The reason why your commit didn't work is that my appveyor.yml runs tox -e py, so the name of the environment is not "py26" nor "py27".

If you make the mock dependency unconditional, it should work everywhere.

@ghost

This comment has been minimized.

Copy link

ghost commented Mar 31, 2016

@mgedmin Got it, thanks. I thought maybe mock wouldn't be available on python 3 since it is now part of unittest, but it appears to work.

objgraph.py Outdated
except AttributeError:
result = repr(value)[:40]

if _isinstance(result, str if sys.version_info[0] == 3 else basestring):

This comment has been minimized.

@mgedmin

mgedmin Apr 1, 2016

Owner

There's a compat redefinition of basestring = str on Python 3 at the top of this file, so this can be simplified to if _isinstance(result, basestring).

objgraph.py Outdated
@@ -65,6 +65,15 @@
iteritems = dict.items


def _isinstance(object, classinfo):
"""Use an implementation of isinstance that doesn't depend on ``__class__``,
which is delegated to the mock spec by the mocking library."""

This comment has been minimized.

@mgedmin

mgedmin Apr 1, 2016

Owner

I try to follow PEP-257 recommendations for docstrings:

  • 1st line is a summary, phrased as a command ("Return whether an object is an instance of a class or subclass.")
  • 2nd line is blank
  • further lines elaborate ("Differs from the builtin isinstance() in that it cannot be fooled by mock.Mock(spec=...)")
  • the closing """ are on a separate line
objgraph.py Outdated
if sys.version_info[0] == 2:
return issubclass(type(object), classinfo)
else:
return isinstance(object, classinfo)

This comment has been minimized.

@mgedmin

mgedmin Apr 1, 2016

Owner

I'm curious why no special handling is required on Python 3.

I'm also tempted to optimize prematurely, i.e.

if sys.version_info[0] >= 3:
    _isinstance = isinstance
else:
    def _isinstance(obj, classinfo):
        """...

        ...
        """
        return issubclass(type(obj), classinfo)
@mgedmin

This comment has been minimized.

Copy link
Owner

mgedmin commented Apr 1, 2016

The rest looks good to me.

Can you help me come up with a changelog entry for this change? I imagine something like

- Fixes a crash in some cases when mocks are involved (GH #26).

but I would prefer to mention the specific exception type (is it a TypeError?). I still haven't seen a traceback

tox.ini Outdated
@@ -6,6 +6,7 @@ commands =
python tests.py {posargs}

[testenv:py]
deps = mock

This comment has been minimized.

@mgedmin

mgedmin Apr 1, 2016

Owner

tox -e py27 also fails for me (on Linux) because it lacks mock.

@mgedmin

This comment has been minimized.

Copy link
Owner

mgedmin commented Apr 1, 2016

For posterity: here's what the various errors look like if I apply the patch to the tests, but run them against unmodified objgraph.py:

======================================================================
ERROR: test_short_repr_mocked_instance_method (__main__.StringRepresentationTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests.py", line 319, in test_short_repr_mocked_instance_method
    self.assertRegex(objgraph._short_repr(my_mock.my_method), '<MagicMock')
  File "/home/mg/src/objgraph/objgraph.py", line 873, in _short_repr
    return obj.im_func.__name__ + ' (bound)'
  File "/usr/lib/python2.7/dist-packages/mock/mock.py", line 718, in __getattr__
    raise AttributeError(name)
AttributeError: __name__

======================================================================
ERROR: test_short_repr_mocked_instance_method_bound (__main__.StringRepresentationTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests.py", line 331, in test_short_repr_mocked_instance_method_bound
    self.assertRegex(objgraph._short_repr(obj.my_method), '<Mock')
  File "/home/mg/src/objgraph/objgraph.py", line 873, in _short_repr
    return obj.im_func.__name__ + ' (bound)'
  File "/usr/lib/python2.7/dist-packages/mock/mock.py", line 718, in __getattr__
    raise AttributeError(name)
AttributeError: __name__

======================================================================
ERROR: test_short_repr_mocked_instance_method_bound_with_mocked_name (__main__.StringRepresentationTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests.py", line 351, in test_short_repr_mocked_instance_method_bound_with_mocked_name
    self.assertRegex(objgraph._short_repr(obj.my_method), '<Mock')
  File "/usr/lib/python2.7/unittest/case.py", line 999, in assertRegexpMatches
    if not expected_regexp.search(text):
TypeError: expected string or buffer

----------------------------------------------------------------------
@ashanbrown

This comment has been minimized.

Copy link
Contributor

ashanbrown commented Apr 1, 2016

I'll work on a change name, but I wanted to mention that both python 2 and python 3 have the behavior:

isinstance(mock.Mock(spec=list), list) == True

Interestingly, the tests all pass now if I revert _isinstance back to isinstance on all python versions so I'll have to investigate this further. I do still believe we should always be using issubclass(type(m), (<types>)) but I'd like to create the tests to prove it.

@ashanbrown

This comment has been minimized.

Copy link
Contributor

ashanbrown commented Apr 5, 2016

I've added a test for mock.Mock(spec=...) that proves that we do need a different definition of _isinstance. I can squash all the above commits, but I thought you might just want to see the diffs.

objgraph.py Outdated
@@ -65,6 +65,17 @@
iteritems = dict.items


def _isinstance(object, classinfo):
""" Return whether an object is an instance of a class or of a subclass
thereof.

This comment has been minimized.

@mgedmin

mgedmin Apr 5, 2016

Owner

This should be on a single line without a space after the opening """.

objgraph.py Outdated
""" Return whether an object is an instance of a class or of a subclass
thereof.
Differs from the builtin python2 isinstance() implementation in that it

This comment has been minimized.

@mgedmin

mgedmin Apr 5, 2016

Owner

You can drop the 'python2', since this is now also used on Python 3.

objgraph.py Outdated
else:
return obj.__func__.__name__
func = obj.__func__
instance = obj.__self__
except AttributeError: # pragma: nocover
# Python < 2.6 compatibility

This comment has been minimized.

@mgedmin

mgedmin Apr 5, 2016

Owner

You can simplify this by dropping the branch that attempts to support Python 2.5 and older.

@mgedmin

This comment has been minimized.

Copy link
Owner

mgedmin commented Apr 5, 2016

Thank you!

Other than the minor docstring formatting issues, this looks good.

(I haven't made up my mind if I prefer squashed commits or a true history of the changes. Either works fine.)

@mgedmin

This comment has been minimized.

Copy link
Owner

mgedmin commented Apr 5, 2016

I tried to trigger a failure with unpatched objgraph (so I can write a good changelog message), but failed.

Here's what I tried:

$ python
Python 2.7.10 (default, Oct 14 2015, 16:09:02) 
[GCC 5.2.1 20151010] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import mock
>>> not_a_real_list = mock.Mock(spec=list)
>>> import objgraph
>>> objgraph.show_refs([not_a_real_list])
Graph written to /tmp/objgraph-wRJIjm.dot (39 nodes)
Spawning graph viewer (xdot)

Here's what I got:
ekrano nuotrauka is 2016-04-05 12-47-39

No crash.

Similarly, including plain mock.Mock() or mock.Mock(__name__=mock.Mock()) failed to induce crashes.

Can you help me find a crashing example?

Use type instead of __class__ to get object class.
This supports graphing unittest mock objects.  Also, ensure that _short_repr always returns a string.
@ashanbrown

This comment has been minimized.

Copy link
Contributor

ashanbrown commented Apr 7, 2016

I've made the changes you suggested and squashed the commits. Here's an example that crashes:

objgraph.show_refs([types.MethodType(mock.Mock(__name__=mock.Mock()), None, list)])

I wish I could explain how I ended up with a unbound instance method with a name that happened to be a mock, but I'm afraid I can't.

For your changelog, I'd say the theme of these changes is that, in the presence of mocks, "don't trust __class__ to tell you the class" and "don't trust __name__ to always give you a string".

@mgedmin mgedmin merged commit 54c04a3 into mgedmin:master Apr 13, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mgedmin

This comment has been minimized.

Copy link
Owner

mgedmin commented Apr 13, 2016

Thank you very much for your patience!

@mgedmin

This comment has been minimized.

Copy link
Owner

mgedmin commented Apr 13, 2016

I've released objgraph 3.0.0 with this fix.

@ashanbrown

This comment has been minimized.

Copy link
Contributor

ashanbrown commented Apr 13, 2016

Excellent. Thank you for guiding me through getting the change right.

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