support graphing mocked methods

[ashanbrown]

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]

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]

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]

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

[ashanbrown]

@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]

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]

@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.

[mgedmin]

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

[mgedmin]

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]

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]

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.

[mgedmin]

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]

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:

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?

[ashanbrown]

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]

Thank you very much for your patience!

[mgedmin]

I've released objgraph 3.0.0 with this fix.

[ashanbrown]

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