Demonstrating how bad the wrapped message is, they should be the same

[txomon]

This test demonstrates how bad those assert wraps in the library can be.

Is totally misleading and displays incomplete information.

The output of the test so that you understand the big difference:

E       assert 'Expected cal...ock(a=1, b=2)' == 'assert () == ...()\n  + (1, 2)'
E         - Expected call: mock(1, 2)
E         - Actual call: mock(a=1, b=2)
E         + assert () == (1, 2)
E         +   Right contains more items, first extra item: 1
E         +   Full diff:
E         +   - ()
E         +   + (1, 2)

As you can see, the actual message (marked with + in the diff) doesn't speak at all about the keyword arguments, and although the diff may be more verbose all the content there is terrible.

This needs to improve by either making:

1. Both messages the same
2. Improve it until all the data appears in the assert message

IMO the assert wrapper function should just disappear, but up to you.

EDIT: Instead of creating the exception again, I would really raise the existing one within the assert wrapper

EDIT2: I think it would also be nice to know why the assert wrapper is necessary at all...

EDIT3: Just in case, tests fail on purpose.

[nicoddemus]

Hey @txomon, thanks for the feedback!

Those wrappers were introduced in #36, perhaps @asfaltboy and others could chime in? I see your example showcases a pretty bad message because the mocks were called with only keyword arguments but expected to be called with positional arguments but I agree it could be improved.

I will take a closer look tomorrow, right now I have to go to bed. 😁

[asfaltboy]

Hi guys, I clearly see the output is intolerable, it's totally my fault for not following through with the improvement suggestion I raised in original PR #36 .

However, I wrote a quick fix in commit 86e03ea, that visibly appends the "pytest introspected" message to the end of the original mock error message so that both "simple" and detailed errors are visible.
In addition, I updated wrapping pytest assertion to compare a tuple of both (args, kwargs) in order for the introspection to display both in the diff.

The new branch I reference above also contains a few fixes for tests and tox.ini (since we now need to import mock in our tests).

@txomon if you can, please try the descriptive-mock-assert-wrap branch and let us know if it works for you.

[txomon]

I won't be able to test it in the next days (will be on vacations without
Internet), but your description is just what I had in mind.

I have seen the code and is there any reason to create the AssetionError in
the wrapper rather than raising it directly?

Cheers!

On Wed, 3 Aug 2016 18:30 Pavel Savchenko, notifications@github.com wrote:

Hi guys, I clearly see the output is intolerable, it's totally my fault
for not following through with the improvement suggestion I raised in
original PR #36 #36 .

However, I wrote a quick fix in commit 86e03ea
86e03ea,
that visibly appends the "pytest introspected" message to the end of the
original mock error message so that both "simple" and detailed errors are
visible.
In addition, I updated wrapping pytest assertion to compare a tuple of
both (args, kwargs) in order for the introspection to display both in the
diff.

The new branch I reference above also contains a few fixes for tests and
tox.ini (since we now need to import mock in our tests).

@txomon https://github.com/txomon if you can, please try the
descriptive-mock-assert-wrap
https://github.com/asfaltboy/pytest-mock/tree/feature/descriptive-mock-assert-wrap
branch and let us know if it works for you.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#57 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAN7mgJdn_Pyy_cwW4TTl9lBVhXaBdanks5qcGOkgaJpZM4JbLsC
.

[asfaltboy]

@txomon do you mean why don't we change this raise statement from

raise AssertionError(*e.args)

to

raise e

?

Honestly I don't know why not, it's legacy from #27 I think

[txomon]

Yes, that way the message is either maintained or improved

On Wed, 3 Aug 2016 20:08 Pavel Savchenko, notifications@github.com wrote:

@txomon https://github.com/txomon do you mean why don't we change this
raise statement
https://github.com/pytest-dev/pytest-mock/blob/master/pytest_mock.py#L164
from

raise AssertionError(*e.args)

to

raise e

?

Honestly I don't know why not, it's legacy from #27
#27 I think

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#57 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAN7mjzBy2GtZdYIBG3n5SMOzGm2-t3aks5qcNgngaJpZM4JbLsC
.

[nicoddemus]

Hey @asfaltboy, thanks a lot for joining the discussion so quickly, and already volunteering to work on it to boot! 😄

About your patch, I think that's the right direction although I'm not a big fan of printing the message like you did, I would rather we add to the original message and raise a new AssertionError:

    __tracebackhide__ = True
    try:
        __wrapped_mock_method__(*args, **kwargs)
    except AssertionError as e:
        __mock_self = args[0]
        if __mock_self.call_args is not None:
            actual_args, actual_kwargs = __mock_self.call_args
            try:
                assert (args[1:], kwargs) == (actual_args, actual_kwargs)
            except AssertionError as detailed_comparison:
                msg = (str(e) + "\n\n... pytest introspection follows:\n" +
                       str(detailed_comparison))
                raise AssertionError(msg)
        raise 

This way we have all the information in the same place, instead of capturing stdout and displaying it in a separate section of the error report.

[asfaltboy]

This is a bit over my head, I mean I'm not sure what the differences / trade-offs are of the above mentioned approaches of re-raising the error, beyond coding style that is.

In any case, I'm happy to comply with any requests and will submit a PR if everyone's happy with the code in the branch.

EDIT: @nicoddemus I just noticed I left in a print() statement I used for own debugging, my bad, removed in the PR I just opened.

[txomon]

@asfaltboy Basically, reraising an exception is cheaper because the object is already there, and most of all, you don't need to know how that exception is built or extra arguments it might need.

Finally, composing strings by addition is not recommended

    __tracebackhide__ = True
    try:
        __wrapped_mock_method__(*args, **kwargs)
    except AssertionError as e:
        __mock_self = args[0]
        if __mock_self.call_args is not None:
            actual_args, actual_kwargs = __mock_self.call_args
            try:
                assert (args[1:], kwargs) == (actual_args, actual_kwargs)
            except AssertionError as detailed_comparison:
                msg = '{exception}\n\n... pytest introspection follows:\n{comparison}'.format(
                    exception=str(e), comparison=str(detailed_comparison)
                }
                raise AssertionError(msg)
        raise 

Notice that those str() calls are to avoid __repr__ being called, but rather __str__ =)

[Chronial]

@asfaltboy I did

raise AssertionError(*e.args)

instead of raise e to get a clean traceback starting at that line without any reference to the old exception. I don't know if that is still useful,

[asfaltboy]

Thanks @Chronial this seems to fix the failing assert_traceback test

[nicoddemus]

Discussion continued in #58