bpo-36871: Handle spec errors in assert_has_calls

[sfreilich]

The fix in PR #13261 handled the underlying issue about the spec for specific methods not being applied correctly, but it didn't fix the issue that was causing the misleading error message.

The code currently grabs a list of responses from _call_matcher (which may include exceptions). But it doesn't reach inside the list when checking if the result is an exception. This results in a misleading error message when one of the provided calls does not match the spec.

https://bugs.python.org/issue36871

Automerge-Triggered-By: @gpshead

[sfreilich]

Ah, wait, I made my fix to the wrong line!

Also, assert_has_awaits has the exact same bug.

[sfreilich]

This error handling bug isn't a huge problem most of the time. But when there's a bug in the creation or application of the mock specs (like the bugs fixed in earlier PRs for this issue), the resulting error message is really confusing, since it incorrectly says that the expected calls were not present, even when the expected and actual calls are in fact identical.

[lisroach]

I feel like I'm missing some context re MultiError, what is that?

It's a PEP currently being written, it would help handle multiple exceptions perfectly.
Previously when encountered with a problem like this I do default to just the last exception raised.

It is interesting from works, seems like the documentation says it must be an Exception type but it is not enforced anywhere. I would err on the side of not using a list in that case, as someone might decide thats a bug someday and "fix" it.

[sfreilich]

do default to just the last exception raised

I think first makes a little more sense. The first exception encountered is the one that changes the state from passing to failing, and it's the one that would be raised if handling of those exceptions was not deferred.

[sfreilich]

Is this ready to merge?

[gpshead]

I think this is ready to merge, but can you use blurb to add a news entry about the fixed error message? It's worth calling out.

[miss-islington]

@sfreilich: Status check is done, and it's a failure ❌ .

[miss-islington]

I'm having trouble backporting to 3.8. Reason: 'Error 110 while writing to socket. Connection timed out.'. Please retry by removing and re-adding the needs backport to 3.8 label.

[miss-islington]

Sorry, @sfreilich, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker b5a7a4f0c20717a4c92c371583b5521b83f40f32 3.7

[sfreilich]

I'll create another PR for that, sorry.

[tirkarthi]

No problem. Please also add a test too with the PR as below which would fail without the error message change suggested. Normally we don't added very specific asserts for error message but I think it helps in this case. I think assert_has_awaits is not affected since it doesn't use self._calls_repr.

with self.assertRaisesRegex(
        AssertionError,
        re.escape("Error processing expected calls.\n"
                  "Errors: [None, TypeError('too many positional "
                  "arguments')]\n"
                  "Expected: [call(), call('wrong')]")) as cm:
    mock.assert_has_calls([call(), call('wrong')])

with self.assertRaisesRegex(
        AssertionError,
        re.escape("Error processing expected calls.\n"
                  "Errors: [None, TypeError('too many positional "
                  "arguments')]\n"
                  "Expected: [call(), call('wrong')]\n"
                  "Actual: [call()]")) as cm:
    mock()
    mock.assert_has_calls([call(), call('wrong')])

[sfreilich]

PR #16361

[miss-islington]

Thanks @sfreilich for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

[miss-islington]

Sorry @sfreilich, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker b5a7a4f0c20717a4c92c371583b5521b83f40f32 3.8

[gpshead]

so much for automation. i'll take care of the backports of both of these PRs.

[miss-islington]

Thanks @sfreilich for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

[bedevere-bot]

GH-16364 is a backport of this pull request to the 3.8 branch.

[tirkarthi]

I am not sure of the backport to 3.7 . Though this fixes the bug in detecting Exception it also enhances and changes error message which some package might be asserting in their test. IMO we can backport only exception detecting code if needed to 3.7 instead of error message enhancement.

[sfreilich]

might be asserting in their test

The specific error message produced by unittest.mock.Mock.assert_has_calls seems like a strange thing to be asserting about in tests. Unless the test is testing a subclass of mock.Mock or something. Basically, I see your concern, but I think the tradeoffs are less towards interface stability when the error messages in question are generated by test assertions.

That said, the more important part is the one that prevents the exceptions from spec.bind from being swallowed entirely and ensures at lest the first one ends up reported to the user somehow (e.g. by setting it as the cause of the AssertionError).

[tirkarthi]

There are packages that assert exact error messages that are mostly built on top of mock module. Sample issue over 3.8 error message alignment change : pytest-dev/pytest-mock#139

That said, the more important part is the one that prevents the exceptions from spec.bind from being swallowed entirely and ensures at lest the first one ends up reported to the user somehow (e.g. by setting it as the cause of the AssertionError).

Okay, I see it as a partly improvement in error message with the bug fix to detect the exception object. I will leave it to the core dev for the backport to 3.7 decision.

[gpshead]

PR #16374 has a backport to 3.7.

I see the incorrect error message as a bug so I'm leaning towards merging it.

That pytest has some of its own test suite depend on specifics of error message isn't super surprising given the nature of pytest, but it is within its own test suite and they seem to understand how to fix fragile golden value data issues in those tests if/when they do come up.

I wouldn't expect any normal actual user code to reasonably depend on specific formatting of error messages from mock assertions.