Feature: descriptive mock assert wrap by asfaltboy · Pull Request #58 · pytest-dev/pytest-mock (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation21 Commits11 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

asfaltboy

This branch adds original call assertion error message and improves result message. Here is a detailed list of changes:

Attempts to improve on PR #36 according to the points raised in PR #57

@txomon @asfaltboy

@asfaltboy

This improves on PR pytest-dev#36 according to the points raised in PR pytest-dev#57

@asfaltboy

@asfaltboy

@asfaltboy

@asfaltboy

@asfaltboy

Ok so re-raising the exception implicitly doesn't work (see failed CI run), and raising e explicitly works, but changes expected location of crash in traceback test.

I opted to writing a custom pytest_assertrepr_compare hook for asserting a Call and CallList comparison. It's a bit gnarly to look at, but has a few advantages at the previous naive approach:

I feel this is pretty much ready for review, however I have an issue with the assert_traceback test, which now fails in py3. Any ideas on how to safely fix this?

@nicoddemus

Hey @asfaltboy,

First of all let me say that I really appreciate all the effort you put into this. 👍

But I have to be honest, I feel a little uneasy on introducing this type of arguably complex code to pytest-mock, plus the current PR uses some internal pytest objects which are not guaranteed to be stable across pytest releases.

For those reasons I was hoping we could use the same message raised by pytest's assertion and just augment the original message from mock.

I played around a bit and came up with this:

def assert_wrapper(wrapped_mock_method, *args, **kwargs): tracebackhide = True try: wrapped_mock_method(*args, **kwargs) return except AssertionError as e: __mock_self = args[0] msg = str(e) if __mock_self.call_args is not None: actual_args, actual_kwargs = __mock_self.call_args try: assert actual_args == args[1:] except AssertionError as e: msg += '\nargs introspection:\n' + str(e) try: assert actual_kwargs == kwargs except AssertionError as e: msg += '\nkwargs introspection:\n' + str(e) assert 0, msg

Which outputs this:

================================== FAILURES ===================================
___________________ test_assertion_error_is_not_descriptive ___________________

mocker = <pytest_mock.MockFixture object at 0x039A91F0>

    def test_assertion_error_is_not_descriptive(mocker):
        """Demonstrate that assert_wrapper does really bad things to assertion messages"""
        mocker_mock = mocker.patch('os.remove')

        mocker_mock(a=1, b=2)
>       mocker_mock.assert_called_once_with(1, 2)
E       AssertionError: Expected call: remove(1, 2)
E       Actual call: remove(a=1, b=2)
E       args introspection:
E       assert () == (1, 2)
E         Right contains more items, first extra item: 1
E         Use -v to get the full diff
E       kwargs introspection:
E       assert {'a': 1, 'b': 2} == {}
E         Left contains more items:
E         {'a': 1, 'b': 2}
E         Use -v to get the full diff
E       args introspection:
E       assert () == (1, 2)
E         Right contains more items, first extra item: 1
E         Use -v to get the full diff
E       kwargs introspection:
E       assert {'a': 1, 'b': 2} == {}
E         Left contains more items:
E         {'a': 1, 'b': 2}
E         Use -v to get the full diff

As you can see unfortunately the assertion introspection messages are duplicated, which I suspect is due to pytest's assertion reinterpretation kicking in...

@The-Compiler

As you can see unfortunately the assertion introspection messages are duplicated, which I suspect is due to pytest's assertion reinterpretation kicking in...

Have you tried with the newest features branch, where the rewriting is gone anyways?

@asfaltboy

Hi @nicoddemus thanks for having a look so quick. I know the pytest_assertrepr_compare hook is daunting, and you have some good points. Let's break the changes down a bit:

  1. I'm using a hook for mock.call objects comparison. I feel it's nicer to leave the object comparison there rather than in our except clause. - agree ?
  2. I don't like using the _pytest internals myself, but I do like the way pytest outputs the assertion diff, and would like to stay close to it.
    Maybe @pytest-dev can comment on the availability of a public-stable API of (at least) assertion.util to plugin developers? This is a feature us devs can use to easily diff deeply nested parts of custom objects and remain consistent with pytest built-in assertion output.
    Worst case scenario, we can "temporarily" use raising/catching args/kwargs as in your snippet, without using util helpers, until a cleaner solution is available in pytest. - Is that an acceptable workaround?
  3. Your example above is missing the assert_any_call. Here we just output the CallList using "pretty formats" so all calls can be easily read (otherwise the "representation" of CallList is limited to maxsize in "summary"). - are you ok with this?

@asfaltboy

@asfaltboy

@asfaltboy

@asfaltboy

I added some commits to prevent dupe message update, address point 2 above and fix the failing assert_traceback test.

@coveralls

Coverage Status

Coverage decreased (-0.6%) to 99.408% when pulling c0382c9 on asfaltboy:feature/descriptive-mock-assert-wrap into fb401b1 on pytest-dev:master.

@nicoddemus

Hey @asfaltboy, just stopping by to comment that I'm a little busy at the moment with the pytest-3.0 release. After that is done I plan to take a closer look at your PR! 😅

Again I really appreciate your efforts on this!

@asfaltboy

@asfaltboy

@coveralls

Coverage Status

Coverage decreased (-0.6%) to 99.415% when pulling 3c9a4f9 on asfaltboy:feature/descriptive-mock-assert-wrap into fb401b1 on pytest-dev:master.

@asfaltboy

Using c0382c9 , non-ascii characters in asserted arguments would break pytest-mock in a nasty UnicodeError exception. The latest commit works around this by safely decoding to unicode the parts that make up the output.

Unfortunately, I am not aware of any clean way to do this without adding dependencies, so I opted to use py.builtin._totext, but we can use six or copy-paste _totext to do this. Please let me know if you want me to use some other method.

Unicode! Phew.

@nicoddemus

Hey @asfaltboy,

First of all sorry for taking so long!

I gave another shot to my patch earlier, which tried to use pytest's own assertion introspection, and discovered what was going on: mock.assert_called_once_with internally calls mock.assert_called_with, which explains the duplicated message. 😅

I updated my patch as follows:

def assert_wrapper(wrapped_mock_method, *args, **kwargs): tracebackhide = True try: wrapped_mock_method(*args, **kwargs) return except AssertionError as e: if getattr(e, '_mock_introspection_applied', 0): raise AssertionError(e) __mock_self = args[0] msg = str(e) if __mock_self.call_args is not None: actual_args, actual_kwargs = __mock_self.call_args msg += '\n\n... pytest introspection follows:\n' try: assert actual_args == args[1:] except AssertionError as e: msg += '\nArgs:\n' + str(e) try: assert actual_kwargs == kwargs except AssertionError as e: msg += '\nKwargs:\n' + str(e) e = AssertionError(msg) e._mock_introspection_applied = True raise e

I mark the exception with a _mock_introspection_applied attribute so when we get the same exception in the other wrapper we don't try to add introspection information again.

Here's the output:

================================== FAILURES ===================================
___________________ test_assertion_error_is_not_descriptive ___________________

mocker = <pytest_mock.MockFixture object at 0x0361C0F0>

    def test_assertion_error_is_not_descriptive(mocker):
        """Demonstrate that assert_wrapper does really bad things to assertion messages"""
        mocker_mock = mocker.patch('os.remove')

        mocker_mock(a=1, b=2)
>       mocker_mock.assert_called_once_with(1, 2)
E       AssertionError: Expected call: remove(1, 2)
E       Actual call: remove(a=1, b=2)
E       
E       ... pytest introspection follows:
E       
E       Args:
E       assert () == (1, 2)
E         Right contains more items, first extra item: 1
E         Use -v to get the full diff
E       Kwargs:
E       assert {'a': 1, 'b': 2} == {}
E         Left contains more items:
E         {'a': 1, 'b': 2}
E         Use -v to get the full diff

X:\pytest-mock\test_foo.py:6: AssertionError
========================== 1 failed in 0.02 seconds ===========================

I think this looks like what we wanted, and IMHO very simple. @asfaltboy what do you think?

Also, @txomon what do you think of this output?

@txomon

This is by far, better than I expected!

@asfaltboy

Hi @nicoddemus and thanks for coming back to this.

The PR code has duplication prevention test since commit 2296697. I did address your initial concern and removed all external uses for the comparison, excepting the py.builtin._totext wrapper, for cross-version unicode support. However, I still feel the comparison logic should be extracted out of assert_wrapper per separation of concerns. We can switch from the use of pytest_assertrepr_compare to a simple function call, it may be easier to follow.

The code in the PR also includes additional features not found in your patch, specifically:

If you think we've cluttered the original issue, I can cleanup this PR to only address the original issue (include original assert and describe args/kwargs), and create new PRs to discuss separately.

@nicoddemus

Hey @asfaltboy, sorry for the delay, been really busy lately!

I agree with most of what you have said, just some comments below:

With the use of pytest_assertrepr_compare, individual Call objects can be compared directly by the user, even without using the assert_call_x methods.

Not really sure how useful that is, but on the other hand I don't remember a case where I had to compare Call instances with large dicts or lists... but I guess that is the whole point of your initial take at improving this. 😁

I say let's keep pytest_assertrepr_compare... we can change this later if need be.

If you think we've cluttered the original issue, I can cleanup this PR to only address the original issue (include original assert and describe args/kwargs), and create new PRs to discuss separately.

I don't think that's necessary... if you can somehow organize this in separate and clean commits though, it would be great.

Also, I think we need to update the CHANGELOG and README.

Again, sorry for the delay and thanks for all your work on this!

@asfaltboy

Hi @nicoddemus thanks for your kind words, and sorry for late response.

I'll try my best to find time to work on cleaning up into meaningful commits and adding docs.

Meanwhile, I've been using this daily and I sometime encounter odd behavior.
Specifically, if I enter pdb on fail, e.g with --pdb, the interactive pdb shell is created after a unittest clean up.
I have not looked into converting into a simple functional test and using py.test cleanup instead. Perhaps this would always happen and is not specific to our traceback monkeypatch.

@nicoddemus

Hi @nicoddemus thanks for your kind words, and sorry for late response.

No worries!

I'll try my best to find time to work on cleaning up into meaningful commits and adding docs.

Thanks, take your time!

Specifically, if I enter pdb on fail, e.g with --pdb, the interactive pdb shell is created after a unittest clean up.

Which pytest version are you using? There's been some changes regarding --pdb with TestCase subclasses in 3.0.2.

@asfaltboy

True! Was using 2.9 something, upgrade to 3.0.3 fixed the issue!

@fogo

Sorry for commenting only after so much was already done, but I have a question for you guys. Isn't this proposal more suited for mock itself than pytest-mock?

I know it is quicker to implement and have it available in pytest-mock however adding this much complexity to this plugin might also be harmful in the long run.

@nicoddemus

I know it is quicker to implement and have it available in pytest-mock however adding this much complexity to this plugin might also be harmful in the long run.

Hmmm @fogo has a strong point here. The original functionality was added here mostly because it made sense to re-use pytest's assertion rewriting, but it has grown beyond that and now implements almost all the diff logic itself. I think the entire community can benefit from this instead.

What do you think @asfaltboy?

@asfaltboy

(EDITED) Sorry for the late response. While it's true that mock users would benefit from this, and I think it's a good idea to open an issue for mock to improve their output, I really didn't implement any diffing here.

All the pretty printing of deeply nested dicts and such is pytest introspect as implemented in #36 . This PR was originally simply an improvement of #36 that was requested in #57.

The functionality we get out of these two PRs is to have the pretty printing of pytest when asserting mock calls. If we want this in mock we have to copy paste the stuff in _pytest/py internals, which pretty print the diff, I'll open an issue and see what they say. If it was up to me, pytest would be part of the python standard lib as well 😄

Aha, an issue for better output for mock call asserts has already been raised. I've added a comment in that issue thread. What's really interesting is that the issue demonstrates that the "nice diff output" of unittest might be enough here. I wonder ...

Alternatively, if we really don't want this in pytest-mock, I can probably extract these features into a separate plugin.

@nicoddemus

@asfaltboy thanks for the followup.

Let's give continue the work on this PR then. As you mention, it is an improvement over #36.

Thanks again for all your work and patience!

@nicoddemus

@asfaltboy I'm closing this for now so the PR doesn't stay open. Feel free to reopen it when/if you get chance to work on this again. Perhaps it might be a good idea to write a separate plugin for it as well like you suggested.

I want to thank you again for all the hard work and interest in contributing to the project! 👍

@asfaltboy asfaltboy deleted the feature/descriptive-mock-assert-wrap branch

December 11, 2018 07:56

@asfaltboy asfaltboy restored the feature/descriptive-mock-assert-wrap branch

December 11, 2018 07:56

@asfaltboy asfaltboy deleted the feature/descriptive-mock-assert-wrap branch

October 22, 2022 10:01