Skip to content

Conversation

yesthesoup
Copy link
Contributor

I made this PR in the past #224 which worked on an if/else for the spy feature to get around a bug in python that was fixed in 3.7. Now that this library supports only >= 3.8 this check is no longer needed.

I suppose this would be "breaking changes" for anyone who may be incorrectly calling methods on class/static method spies?

@nicoddemus
Copy link
Member

Hey @yesthesoup, thanks for the follow up!

I suppose this would be "breaking changes" for anyone who may be incorrectly calling methods on class/static method spies?

To confirm, would any Python 3.8+ user be affected by this change in any way?

@yesthesoup
Copy link
Contributor Author

yesthesoup commented Dec 6, 2023

Hey @yesthesoup, thanks for the follow up!

I suppose this would be "breaking changes" for anyone who may be incorrectly calling methods on class/static method spies?

To confirm, would any Python 3.8+ user be affected by this change in any way?

I believe they would only be affected if their tests were silently passing when they shouldn't be like in the examples in the Python docs:

https://docs.python.org/3/library/unittest.mock.html#auto-speccing

mock = Mock(name='Thing', return_value=None)
mock(1, 2, 3)
mock.assret_called_once_with(4, 5, 6)  # would not throw error in their current tests - silently pass

e.g. if they had some class method / static method spies that pytest-mock was previously creating with autospec=false due to the older python versions' bug and those spies had incorrectly typed methods called on them

Should be a very small percentage of users, maybe none, and if this is merged, it would probably be good for those affected to update and have those silent failures fixed.

@nicoddemus
Copy link
Member

I see, thanks! I guess we might leave out this from the CHANGELOG then for now, and if somebody is affected by this, we can add a note accordingly.

Thanks again!

@nicoddemus nicoddemus merged commit 7f9c131 into pytest-dev:main Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants