Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

autospec for spy: remove if else which was in place for python < 3.7 #399

Merged
merged 2 commits into from
Dec 6, 2023

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
16 checks passed
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