fix(code_mappings): Files that do not match the package name should not match #40892
Conversation
github-actions
bot
added
the
Scope: Backend
| self.dir_path = "" | ||
| self.file_name = self.file_and_dir_path | ||
| else: | ||
| self.root = "" | ||
| self.dir_path = "" |
There was a problem hiding this comment.
This was a bug and it is now tested via test_frame_filename_* tests.
| @@ -123,6 +125,16 @@ def _find_code_mapping(self, frame_filename: FrameFilename) -> Union[CodeMapping | |||
|
|
|||
| return _code_mappings[0] | |||
|
|
|||
| def _get_code_mapping_source_path(self, src_file: str, frame_filename: FrameFilename) -> str: | |||
There was a problem hiding this comment.
This adds the NotImplementedError functionality.
| e.g. ssl.py -> raise NotImplemented | ||
| """ | ||
| if frame_filename.dir_path != "": | ||
| return src_file.rsplit(frame_filename.dir_path)[0].rstrip("/") |
There was a problem hiding this comment.
This is the original logic. See down below:
- source_path=matched_files[0].rsplit(frame_filename.dir_path)[0].rstrip("/"),| @@ -140,8 +152,9 @@ def _generate_code_mapping_from_tree( | |||
| CodeMapping( | |||
| repo=self.trees[repo_full_name].repo, | |||
| stacktrace_root=frame_filename.root, # sentry | |||
| # e.g. src/sentry/identity/oauth2.py -> src/sentry | |||
| source_path=matched_files[0].rsplit(frame_filename.dir_path)[0].rstrip("/"), | |||
| source_path=self._get_code_mapping_source_path( | |||
There was a problem hiding this comment.
Using _get_code_mapping_source_path to raise NotImplementedError.
There's logic in _potential_match to prevent matching, however, it is safer to prevent issues during future refactorings.
| assert cm == [] | ||
|
|
||
| def test_no_support_for_toplevel_files(self): | ||
| file_name = "base.py" |
There was a problem hiding this comment.
I want to test all the functions that could potential try to handle a single filename and make sure that they all would prevent trying to process it by mistakes.
This is to prevent that refactorings would allow for this case scenario.
| # - "src/sentry_plugins/slack/client.py", | ||
| # - "src/sentry/integrations/slack/client.py", | ||
| "sentry_plugins/slack/client.py", | ||
| ] | ||
| code_mappings = self.code_mapping_helper.generate_code_mappings(stacktraces) | ||
| assert code_mappings == [ | ||
| CodeMapping( |
There was a problem hiding this comment.
The fix to prevent false positives gives us this new match when before we wouldn't.
| ] | ||
|
|
||
| def test_no_stacktraces_to_process(self): | ||
| code_mappings = self.code_mapping_helper.generate_code_mappings([]) |
There was a problem hiding this comment.
I want to make sure that an empty list would still work.
| ] | ||
|
|
||
| def test_no_stacktraces_to_process(self): | ||
| code_mappings = self.code_mapping_helper.generate_code_mappings([]) | ||
| assert code_mappings == [] | ||
|
|
||
| def test_more_than_one_match_works_when_code_mapping_excludes_other_match(self): |
There was a problem hiding this comment.
This test doesn't make sense but it may prevent some issues in the future.
| assert code_mappings == self.expected_code_mappings | ||
| # Asserting lists with different order of elements would fail, thus, checking indexes | ||
| assert code_mappings[0] == self.expected_code_mappings[1] | ||
| assert code_mappings[1] == self.expected_code_mappings[0] |
There was a problem hiding this comment.
Before it worked because the Reprocessing of stacktraces would allow matching sentry_plugins after the sentry code mappings would be generated.
We could make the call of generate_code_mappings to always return code mappings in the same order. I think this is fine.
Co-authored-by: Snigdha Sharma <16563948+snigdhas@users.noreply.github.com>
For instance,
raven/base.pyshould not think that the fileapostello/views/base.pyshould be the related source file.There's also some other minor tests added.