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

Replace called_with with assert_called_with #42712

Closed
tirkarthi opened this issue Jan 3, 2023 · 5 comments
Closed

Replace called_with with assert_called_with #42712

tirkarthi opened this issue Jan 3, 2023 · 5 comments
Assignees

Comments

@tirkarthi
Copy link

Environment

SaaS (https://sentry.io/)

Version

No response

Link

No response

DSN

No response

Steps to Reproduce

called_with is an incorrect method that just returns a Mock object that evaluates to True and doesn't perform appropriate assertion. This method should be replaced with assert_called_with . Related CPython issue : python/cpython#100690

rg '\.called_with\(' -t py | grep -iv faux      
tests/sentry/auth/test_helper.py:        assert mock_auth.get_login_redirect.called_with(self.request)
tests/sentry/auth/test_helper.py:            assert mock_auth.get_login_redirect.called_with(self.request)
tests/sentry/auth/test_helper.py:        assert mock_messages.add_message.called_with(
tests/sentry/auth/test_helper.py:        assert mock_messages.add_message.called_with(
tests/sentry/tasks/test_derive_code_mappings.py:        assert logger.warning.called_with("The org has uninstalled the Sentry App.")
tests/sentry/tasks/test_derive_code_mappings.py:            assert logger.warning.called_with("derive_code_mappings.getting_lock_failed")
tests/sentry/tasks/test_post_process.py:        assert mock_derive_code_mappings.delay.called_with(self.project.id, data, False)
tests/sentry/tasks/test_post_process.py:        assert mock_derive_code_mappings.delay.called_with(self.project.id, data, True)
tests/sentry/integrations/utils/test_code_mapping.py:        assert logger.warning.called_with("More than one repo matched sentry/web/urls.py")
tests/sentry/integrations/github/test_integration.py:                assert logger.info.called_with(msg)
tests/sentry/integrations/github/test_integration.py:            assert logger.info.called_with("Using cached trees for Test-Organization.")
tests/sentry/notifications/utils/test_tasks.py:        assert mock_delay.called_with(

Expected Result

Use assert_called_with method

Actual Result

called_with method is used.

@getsantry
Copy link
Contributor

getsantry bot commented Jan 3, 2023

Assigning to @getsentry/support for routing, due by Wed Jan 04 2023 01:00:00 GMT+0000. ⏲️

@getsantry
Copy link
Contributor

getsantry bot commented Jan 3, 2023

Routing to @getsentry/team-web-sdk-backend for triage, due by Thu Jan 05 2023 01:00:00 GMT+0000. ⏲️

@getsantry
Copy link
Contributor

getsantry bot commented Jan 4, 2023

Routing to @getsentry/app-backend for triage, due by Fri Jan 06 2023 01:00:00 GMT+0000. ⏲️

@JoshFerge
Copy link
Member

JoshFerge commented Jan 5, 2023

@armenzg I've fixed some of these in #42799, but it looks like some recent tests have this issue and I wasn't able to fix them easily. mind taking a look? at:

tests/sentry/tasks/test_derive_code_mappings.py
tests/sentry/integrations/github/test_integration.py
tests/sentry/tasks/test_post_process.py

JoshFerge added a commit that referenced this issue Jan 5, 2023
fixes the easier broken test assertions reported in
#42712
@armenzg armenzg self-assigned this Jan 5, 2023
armenzg added a commit that referenced this issue Jan 12, 2023
In #42712 it was reported that the usage of `called_with` is not correct and it indeed does not work.

This part of the test is not as important as it used to be. We can remove it.
armenzg added a commit that referenced this issue Jan 12, 2023
… it was not testing what we intended.

Unfortunately, I have not been able to fix it in any of the cases.
armenzg added a commit that referenced this issue Jan 12, 2023
In #42712 it was reported that are usage of `called_with` is incorrect.

Unfortunately, using `assert_called_with` does not work because comparing `NodeData` would require extra work to make the comparison of two objects work well.
armenzg added a commit that referenced this issue Jan 12, 2023
In #42712 it was reported that the usage of `called_with` is not correct and it indeed does not work.

This part of the test is not as important as it used to be. We can remove it.
@armenzg
Copy link
Member

armenzg commented Jan 13, 2023

Handling this in #43199

armenzg added a commit that referenced this issue Jan 13, 2023
In #42712 it was reported that the usage of `called_with` is not correct.

I tried using `assert_called_with` but there were various issues to fix it.
@armenzg armenzg closed this as completed May 1, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants