tests(code_mappings): Remove usage of called_with assertions #43199
Conversation
… it was not testing what we intended. Unfortunately, I have not been able to fix it in any of the cases.
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.
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 this to the Post-GA fixes and improvements for derived code mappings milestone
| @@ -72,10 +70,6 @@ class GitHubIntegrationTest(IntegrationTestCase): | |||
| provider = GitHubIntegrationProvider | |||
| base_url = "https://api.github.com" | |||
|
|
|||
| @pytest.fixture(autouse=True) | |||
| def inject_fixtures(self, caplog): | |||
There was a problem hiding this comment.
This is not needed anymore since we won't do assertions in the logger.
| @@ -643,33 +637,22 @@ def test_get_trees_for_org(self): | |||
| ), | |||
| } | |||
|
|
|||
| with patch("sentry.integrations.utils.code_mapping.logger") as logger: | |||
There was a problem hiding this comment.
This is mostly a left indent since we won't be patching the logger anymore.
| with patch("sentry.integrations.utils.code_mapping.logger") as logger: | ||
| assert not cache.get("githubtrees:repositories:Test-Organization") | ||
| # This allows checking for caching related output | ||
| self._caplog.set_level(logging.INFO, logger="sentry") |
| "The Github App does not have access to Test-Organization/baz.", | ||
| "Caching trees for Test-Organization", | ||
| ]: | ||
| assert logger.info.called_with(msg) |
|
|
||
| # Calling a second time should produce the same results | ||
| trees = installation.get_trees_for_org() | ||
| assert logger.info.called_with("Using cached trees for Test-Organization.") |
| self._call_post_process_group(data) | ||
| assert mock_derive_code_mappings.delay.call_count == 1 | ||
| # Because we only run on dry run mode even if the official flag is set | ||
| assert mock_derive_code_mappings.delay.called_with(self.project.id, data, True) |
There was a problem hiding this comment.
I tried switching to assert_called_with, however, the NodeData object event though it was the same was not handling well comparing one of it's properties. IIRC correctly, a dictionary of properties would not compared to be the same because the key/values were out of order.
Instead of working to fix the comparison I decided to remove it. If we ever change the delay call, we should spend time to change it to not use a NodeData by extracting the information we need (e.g. stacktrace and project info).
There was a problem hiding this comment.
Removing the test seems okay. FYI Python introduced ordering in dictionaries, based on order of insertion as of 3.6 or 3.7.
armenzg
deleted the
armenzg/code-mappings/fix-derive-code-mappings-tests
branch
In #42712 it was reported that the usage of
called_withis not correct.I tried using
assert_called_withbut there were various issues to fix it.