diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 2af04bd3bb4eeb..baa801343e4287 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -507,6 +507,11 @@ def get_blame_for_files( # already part of the depot path we get from stacktrace (SourceLineInfo) depot_path = self.build_depot_path(file.repo, file.path, None) + # If revision is available from symcache, append it to depot path + # This allows us to get the exact changelist that created this revision + if file.revision: + depot_path = f"{depot_path}#{file.revision}" + # Use p4 changes -m 1 -l to get most recent change for this file # -m 1: limit to 1 result (most recent) # -l: include full changelist description @@ -554,6 +559,7 @@ def get_blame_for_files( ref=file.ref, repo=file.repo, code_mapping=file.code_mapping, + revision=file.revision, commit=commit, ) blames.append(blame_info) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 3262275bf5e5d2..52de0be6ee7ea5 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -210,6 +210,22 @@ def source_url_matches(self, url: str) -> bool: return False + def get_stacktrace_link( + self, repo: Repository, filepath: str, default: str, version: str | None + ) -> str | None: + """ + Get stacktrace link for Perforce file. + + For Perforce, version represents the file revision number. + We append it to the filepath using Perforce's #revision syntax. + """ + # Append version/revision to filepath if provided + if version: + filepath = f"{filepath}#{version}" + + # Use parent implementation with the modified filepath + return self.check_file(repo, filepath, default) + def check_file(self, repo: Repository, filepath: str, branch: str | None = None) -> str | None: """ Check if a file exists in the Perforce depot and return the URL. diff --git a/src/sentry/integrations/source_code_management/commit_context.py b/src/sentry/integrations/source_code_management/commit_context.py index b8795de56b8ba3..4755e9bd374977 100644 --- a/src/sentry/integrations/source_code_management/commit_context.py +++ b/src/sentry/integrations/source_code_management/commit_context.py @@ -84,6 +84,7 @@ class SourceLineInfo: ref: str repo: Repository code_mapping: RepositoryProjectPathConfig + revision: str | None @dataclass diff --git a/src/sentry/integrations/utils/commit_context.py b/src/sentry/integrations/utils/commit_context.py index c1366cce36fe2f..3a105ef361c99d 100644 --- a/src/sentry/integrations/utils/commit_context.py +++ b/src/sentry/integrations/utils/commit_context.py @@ -254,6 +254,7 @@ def _generate_integration_to_files_mapping( ref=code_mapping.default_branch or "master", repo=code_mapping.repository, code_mapping=code_mapping, + revision=frame.revision, ) ) break diff --git a/src/sentry/integrations/utils/stacktrace_link.py b/src/sentry/integrations/utils/stacktrace_link.py index b31d3e0870dd42..ab7ea338bd4b3e 100644 --- a/src/sentry/integrations/utils/stacktrace_link.py +++ b/src/sentry/integrations/utils/stacktrace_link.py @@ -103,7 +103,9 @@ def get_stacktrace_config( result["error"] = "stack_root_mismatch" continue - outcome = get_link(config, src_path, ctx["commit_id"]) + # Use commit_id if available, otherwise fall back to revision (from symcache) + version = ctx["commit_id"] if ctx["commit_id"] else ctx.get("revision") + outcome = get_link(config, src_path, version) result["iteration_count"] += 1 result["current_config"] = { diff --git a/src/sentry/interfaces/stacktrace.py b/src/sentry/interfaces/stacktrace.py index 955e2076e3ad7a..96ef8fd83d9616 100644 --- a/src/sentry/interfaces/stacktrace.py +++ b/src/sentry/interfaces/stacktrace.py @@ -161,6 +161,7 @@ def to_python(cls, data, **kwargs): "platform", "post_context", "pre_context", + "revision", "source_link", "symbol", "symbol_addr", @@ -198,6 +199,7 @@ def to_json(self): "lineno": self.lineno, "colno": self.colno, "lock": self.lock, + "revision": self.revision or None, "source_link": self.source_link or None, } ) @@ -233,6 +235,7 @@ def get_api_context(self, is_public=False, platform=None, pad_addr=None): "trust": self.trust, "errors": self.errors, "lock": self.lock, + "revision": self.revision, "sourceLink": source_link, } @@ -291,6 +294,7 @@ def get_meta_context(self, meta, is_public=False, platform=None): "trust": meta.get("trust"), "errors": meta.get("errors"), "lock": meta.get("lock"), + "revision": meta.get("revision"), "sourceLink": meta.get("source_link"), } diff --git a/src/sentry/issues/endpoints/project_stacktrace_link.py b/src/sentry/issues/endpoints/project_stacktrace_link.py index 5d4dae85abc107..13afb52ed461ae 100644 --- a/src/sentry/issues/endpoints/project_stacktrace_link.py +++ b/src/sentry/issues/endpoints/project_stacktrace_link.py @@ -1,7 +1,7 @@ from __future__ import annotations import logging -from typing import TypedDict +from typing import NotRequired, TypedDict from django.http import QueryDict from rest_framework.request import Request @@ -36,6 +36,7 @@ class StacktraceLinkContext(TypedDict): module: str | None package: str | None sdk_name: str | None + revision: NotRequired[str | None] def generate_context(parameters: QueryDict) -> StacktraceLinkContext: @@ -51,6 +52,7 @@ def generate_context(parameters: QueryDict) -> StacktraceLinkContext: "package": parameters.get("package"), "line_no": parameters.get("lineNo"), "group_id": parameters.get("groupId"), + "revision": parameters.get("revision"), } diff --git a/src/sentry/lang/native/processing.py b/src/sentry/lang/native/processing.py index 7a523cd006986c..0a7606efb3a6c7 100644 --- a/src/sentry/lang/native/processing.py +++ b/src/sentry/lang/native/processing.py @@ -119,6 +119,8 @@ def _merge_frame(new_frame, symbolicated, platform="native"): new_frame["post_context"] = symbolicated["post_context"] if symbolicated.get("source_link"): new_frame["source_link"] = symbolicated["source_link"] + if symbolicated.get("revision"): + new_frame["revision"] = symbolicated["revision"] addr_mode = symbolicated.get("addr_mode") if addr_mode is None: diff --git a/src/sentry/utils/event_frames.py b/src/sentry/utils/event_frames.py index e54b04774d4d33..8fd9cf0da80775 100644 --- a/src/sentry/utils/event_frames.py +++ b/src/sentry/utils/event_frames.py @@ -22,6 +22,7 @@ class EventFrame: function: str | None = None package: str | None = None module: str | None = None + revision: str | None = None @classmethod def from_dict(cls, data: Mapping[str, Any]) -> EventFrame: diff --git a/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_basic.pysnap b/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_basic.pysnap index 77e2d4c62bb228..f9d9218ec79585 100644 --- a/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_basic.pysnap +++ b/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_basic.pysnap @@ -28,6 +28,7 @@ get_api_context: package: null platform: null rawFunction: null + revision: null sourceLink: null symbol: null symbolAddr: null @@ -61,6 +62,7 @@ get_api_context: package: null platform: null rawFunction: null + revision: null sourceLink: null symbol: null symbolAddr: null diff --git a/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_mechanism.pysnap b/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_mechanism.pysnap index 0d82895982967f..de07f72b27837d 100644 --- a/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_mechanism.pysnap +++ b/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_mechanism.pysnap @@ -31,6 +31,7 @@ get_api_context: package: null platform: null rawFunction: null + revision: null sourceLink: null symbol: null symbolAddr: null diff --git a/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_mixed_frames.pysnap b/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_mixed_frames.pysnap index 4a8168517915f9..48b5907c20301a 100644 --- a/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_mixed_frames.pysnap +++ b/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_mixed_frames.pysnap @@ -28,6 +28,7 @@ get_api_context: package: null platform: null rawFunction: null + revision: null sourceLink: null symbol: null symbolAddr: null @@ -61,6 +62,7 @@ get_api_context: package: null platform: null rawFunction: null + revision: null sourceLink: null symbol: null symbolAddr: null diff --git a/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_only_app_frames.pysnap b/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_only_app_frames.pysnap index 911284bac1740e..cbd8e52fe01637 100644 --- a/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_only_app_frames.pysnap +++ b/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_only_app_frames.pysnap @@ -28,6 +28,7 @@ get_api_context: package: null platform: null rawFunction: null + revision: null sourceLink: null symbol: null symbolAddr: null @@ -61,6 +62,7 @@ get_api_context: package: null platform: null rawFunction: null + revision: null sourceLink: null symbol: null symbolAddr: null diff --git a/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_only_system_frames.pysnap b/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_only_system_frames.pysnap index 1fdaa42d37842a..c0a0ee9b738b70 100644 --- a/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_only_system_frames.pysnap +++ b/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_only_system_frames.pysnap @@ -28,6 +28,7 @@ get_api_context: package: null platform: null rawFunction: null + revision: null sourceLink: null symbol: null symbolAddr: null @@ -61,6 +62,7 @@ get_api_context: package: null platform: null rawFunction: null + revision: null sourceLink: null symbol: null symbolAddr: null diff --git a/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_raw_values.pysnap b/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_raw_values.pysnap index 4c0c77c324dcd1..b9c1e0b7d85aff 100644 --- a/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_raw_values.pysnap +++ b/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_raw_values.pysnap @@ -25,6 +25,7 @@ get_api_context: package: null platform: null rawFunction: null + revision: null sourceLink: null symbol: null symbolAddr: null @@ -51,6 +52,7 @@ get_api_context: package: null platform: null rawFunction: null + revision: null sourceLink: null symbol: null symbolAddr: null diff --git a/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_symbols.pysnap b/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_symbols.pysnap index fafe7d6de305ba..90ff26e70d3b83 100644 --- a/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_symbols.pysnap +++ b/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_symbols.pysnap @@ -28,6 +28,7 @@ get_api_context: package: null platform: null rawFunction: null + revision: null sourceLink: null symbol: Class.myfunc symbolAddr: null diff --git a/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_two_exceptions_having_mechanism.pysnap b/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_two_exceptions_having_mechanism.pysnap index 5b335677995ccd..0acf95cb6ebf41 100644 --- a/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_two_exceptions_having_mechanism.pysnap +++ b/tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_two_exceptions_having_mechanism.pysnap @@ -33,6 +33,7 @@ get_api_context: package: null platform: null rawFunction: null + revision: null sourceLink: null symbol: null symbolAddr: null @@ -71,6 +72,7 @@ get_api_context: package: null platform: null rawFunction: null + revision: null sourceLink: null symbol: null symbolAddr: null diff --git a/tests/sentry/event_manager/interfaces/snapshots/test_threads/test_basics.pysnap b/tests/sentry/event_manager/interfaces/snapshots/test_threads/test_basics.pysnap index f0aeea3def5a6b..433ea00e77a4f4 100644 --- a/tests/sentry/event_manager/interfaces/snapshots/test_threads/test_basics.pysnap +++ b/tests/sentry/event_manager/interfaces/snapshots/test_threads/test_basics.pysnap @@ -1,5 +1,5 @@ --- -source: tests/sentry/event_manager/interfaces/test_threads.py +source: tests/sentry/event_manager/interfaces/test_threads.py::test_basics --- api_context: values: @@ -30,6 +30,7 @@ api_context: package: null platform: null rawFunction: null + revision: null sourceLink: null symbol: null symbolAddr: null @@ -54,6 +55,7 @@ api_context: package: null platform: null rawFunction: null + revision: null sourceLink: null symbol: null symbolAddr: null diff --git a/tests/sentry/integrations/github/test_client.py b/tests/sentry/integrations/github/test_client.py index b1dd8819dfb976..b4563cf0484dc2 100644 --- a/tests/sentry/integrations/github/test_client.py +++ b/tests/sentry/integrations/github/test_client.py @@ -949,6 +949,7 @@ def setUp(self, get_jwt): ref="master", repo=self.repo, code_mapping=None, # type: ignore[arg-type] + revision=None, ) @@ -973,6 +974,7 @@ def test_get_blame_for_files_same_repo(self, get_jwt) -> None: ref="master", repo=self.repo_1, code_mapping=None, # type: ignore[arg-type] + revision=None, ) file2 = SourceLineInfo( path="src/sentry/integrations/github/client_1.py", @@ -980,6 +982,7 @@ def test_get_blame_for_files_same_repo(self, get_jwt) -> None: ref="master", repo=self.repo_1, code_mapping=None, # type: ignore[arg-type] + revision=None, ) file3 = SourceLineInfo( path="src/sentry/integrations/github/client_2.py", @@ -987,6 +990,7 @@ def test_get_blame_for_files_same_repo(self, get_jwt) -> None: ref="master", repo=self.repo_1, code_mapping=None, # type: ignore[arg-type] + revision=None, ) query = """query ($repo_name_0: String!, $repo_owner_0: String!, $ref_0_0: String!, $path_0_0_0: String!, $path_0_0_1: String!) { repository0: repository(name: $repo_name_0, owner: $repo_owner_0) { @@ -1063,6 +1067,7 @@ def test_get_blame_for_files_different_repos(self, get_jwt) -> None: ref="master", repo=self.repo_1, code_mapping=None, # type: ignore[arg-type] + revision=None, ) file2 = SourceLineInfo( path="src/sentry/integrations/github/client_2.py", @@ -1070,6 +1075,7 @@ def test_get_blame_for_files_different_repos(self, get_jwt) -> None: ref="master", repo=self.repo_1, code_mapping=None, # type: ignore[arg-type] + revision=None, ) file3 = SourceLineInfo( path="src/getsentry/file.py", @@ -1077,6 +1083,7 @@ def test_get_blame_for_files_different_repos(self, get_jwt) -> None: ref="master", repo=self.repo_2, code_mapping=None, # type: ignore[arg-type] + revision=None, ) query = """query ($repo_name_0: String!, $repo_owner_0: String!, $ref_0_0: String!, $path_0_0_0: String!, $path_0_0_1: String!, $repo_name_1: String!, $repo_owner_1: String!, $ref_1_0: String!, $path_1_0_0: String!) { repository0: repository(name: $repo_name_0, owner: $repo_owner_0) { @@ -1181,6 +1188,7 @@ def test_get_blame_for_files_different_refs(self, get_jwt) -> None: ref="master", repo=self.repo_1, code_mapping=None, # type: ignore[arg-type] + revision=None, ) file2 = SourceLineInfo( path="src/sentry/integrations/github/client.py", @@ -1188,6 +1196,7 @@ def test_get_blame_for_files_different_refs(self, get_jwt) -> None: ref="master", repo=self.repo_1, code_mapping=None, # type: ignore[arg-type] + revision=None, ) file3 = SourceLineInfo( path="src/sentry/integrations/github/client.py", @@ -1195,6 +1204,7 @@ def test_get_blame_for_files_different_refs(self, get_jwt) -> None: ref="staging", repo=self.repo_1, code_mapping=None, # type: ignore[arg-type] + revision=None, ) query = """query ($repo_name_0: String!, $repo_owner_0: String!, $ref_0_0: String!, $path_0_0_0: String!, $ref_0_1: String!, $path_0_1_0: String!) { repository0: repository(name: $repo_name_0, owner: $repo_owner_0) { @@ -1278,6 +1288,7 @@ def test_trim_file_path_for_query(self, get_jwt) -> None: ref="master", repo=self.repo_1, code_mapping=None, # type: ignore[arg-type] + revision=None, ) query = """query ($repo_name_0: String!, $repo_owner_0: String!, $ref_0_0: String!, $path_0_0_0: String!) { @@ -1340,6 +1351,7 @@ def setUp(self) -> None: ref="master", repo=self.repo_1, code_mapping=None, # type: ignore[arg-type] + revision=None, ) self.file2 = SourceLineInfo( path="src/sentry/integrations/github/client_1.py", @@ -1347,6 +1359,7 @@ def setUp(self) -> None: ref="master", repo=self.repo_1, code_mapping=None, # type: ignore[arg-type] + revision=None, ) self.file3 = SourceLineInfo( path="src/sentry/integrations/github/client_2.py", @@ -1354,6 +1367,7 @@ def setUp(self) -> None: ref="master", repo=self.repo_1, code_mapping=None, # type: ignore[arg-type] + revision=None, ) self.data = { @@ -1570,6 +1584,7 @@ def test_get_blame_for_files_response_partial_data(self, get_jwt) -> None: ref="master", repo=self.repo_1, code_mapping=None, # type: ignore[arg-type] + revision=None, ) file2 = SourceLineInfo( path="src/sentry/integrations/github/client_2.py", @@ -1577,6 +1592,7 @@ def test_get_blame_for_files_response_partial_data(self, get_jwt) -> None: ref="master", repo=self.repo_1, code_mapping=None, # type: ignore[arg-type] + revision=None, ) file3 = SourceLineInfo( path="src/sentry/integrations/github/client.py", @@ -1584,6 +1600,7 @@ def test_get_blame_for_files_response_partial_data(self, get_jwt) -> None: ref="master", repo=self.repo_2, code_mapping=None, # type: ignore[arg-type] + revision=None, ) file4 = SourceLineInfo( path="src/sentry/integrations/github/client.py", @@ -1591,6 +1608,7 @@ def test_get_blame_for_files_response_partial_data(self, get_jwt) -> None: ref="master", repo=self.repo_3, code_mapping=None, # type: ignore[arg-type] + revision=None, ) data = { "repository0": { @@ -1658,6 +1676,7 @@ def test_get_blame_for_files_invalid_commit(self, get_jwt, mock_logger_error) -> ref="master", repo=self.repo_1, code_mapping=None, # type: ignore[arg-type] + revision=None, ) file2 = SourceLineInfo( path="src/sentry/integrations/github/client_2.py", @@ -1665,6 +1684,7 @@ def test_get_blame_for_files_invalid_commit(self, get_jwt, mock_logger_error) -> ref="master", repo=self.repo_1, code_mapping=None, # type: ignore[arg-type] + revision=None, ) data = { "repository0": { @@ -1761,6 +1781,7 @@ def setUp(self) -> None: ref="master", repo=self.repo_1, code_mapping=None, # type: ignore[arg-type] + revision=None, ) responses.reset() responses.add( diff --git a/tests/sentry/integrations/github/test_integration.py b/tests/sentry/integrations/github/test_integration.py index 590c4bc0fd6538..48831f5a925be3 100644 --- a/tests/sentry/integrations/github/test_integration.py +++ b/tests/sentry/integrations/github/test_integration.py @@ -1126,6 +1126,7 @@ def test_get_commit_context_all_frames(self) -> None: ref="master", repo=repo, code_mapping=None, # type: ignore[arg-type] + revision=None, ) responses.add( diff --git a/tests/sentry/integrations/github_enterprise/test_integration.py b/tests/sentry/integrations/github_enterprise/test_integration.py index 8d541f1c2be02a..3d86f9c02d27c4 100644 --- a/tests/sentry/integrations/github_enterprise/test_integration.py +++ b/tests/sentry/integrations/github_enterprise/test_integration.py @@ -396,6 +396,7 @@ def test_get_commit_context_all_frames(self, _: MagicMock, __: MagicMock) -> Non ref="master", repo=repo, code_mapping=None, # type: ignore[arg-type] + revision=None, ) responses.add( diff --git a/tests/sentry/integrations/gitlab/test_client.py b/tests/sentry/integrations/gitlab/test_client.py index 760af2d4d72ca8..b545fe56392477 100644 --- a/tests/sentry/integrations/gitlab/test_client.py +++ b/tests/sentry/integrations/gitlab/test_client.py @@ -370,6 +370,7 @@ def setUp(self) -> None: ref="master", repo=self.repo, code_mapping=None, # type: ignore[arg-type] + revision=None, ) self.file_2 = SourceLineInfo( path="src/sentry/integrations/github/client_1.py", @@ -377,6 +378,7 @@ def setUp(self) -> None: ref="master", repo=self.repo, code_mapping=None, # type: ignore[arg-type] + revision=None, ) self.file_3 = SourceLineInfo( path="src/sentry/integrations/github/client_2.py", @@ -384,6 +386,7 @@ def setUp(self) -> None: ref="master", repo=self.repo, code_mapping=None, # type: ignore[arg-type] + revision=None, ) self.file_4 = SourceLineInfo( path="src/sentry/integrations/github/client_3.py", @@ -391,6 +394,7 @@ def setUp(self) -> None: ref="master", repo=self.repo, code_mapping=None, # type: ignore[arg-type] + revision=None, ) self.blame_1 = FileBlameInfo( **asdict(self.file_1), diff --git a/tests/sentry/integrations/gitlab/test_integration.py b/tests/sentry/integrations/gitlab/test_integration.py index efe63b62c351b7..9253585a0aa15c 100644 --- a/tests/sentry/integrations/gitlab/test_integration.py +++ b/tests/sentry/integrations/gitlab/test_integration.py @@ -391,6 +391,7 @@ def test_get_commit_context_all_frames(self) -> None: ref="master", repo=repo, code_mapping=None, # type: ignore[arg-type] + revision=None, ) responses.add( diff --git a/tests/sentry/integrations/perforce/test_client.py b/tests/sentry/integrations/perforce/test_client.py index a2152984bb25e1..2235a858aa89c1 100644 --- a/tests/sentry/integrations/perforce/test_client.py +++ b/tests/sentry/integrations/perforce/test_client.py @@ -78,6 +78,7 @@ def test_get_blame_for_files_success(self, mock_p4_class): ref="", repo=self.repo, code_mapping=None, # type: ignore[arg-type] + revision=None, ) file2 = SourceLineInfo( path="src/utils.py", @@ -85,6 +86,7 @@ def test_get_blame_for_files_success(self, mock_p4_class): ref="", repo=self.repo, code_mapping=None, # type: ignore[arg-type] + revision=None, ) blames = self.p4_client.get_blame_for_files([file1, file2], extra={}) @@ -124,6 +126,7 @@ def test_get_blame_for_files_no_changelist(self, mock_p4_class): ref="", repo=self.repo, code_mapping=None, # type: ignore[arg-type] + revision=None, ) blames = self.p4_client.get_blame_for_files([file], extra={}) @@ -161,6 +164,7 @@ def test_get_blame_for_files_with_stream(self, mock_p4_class): ref="main", # Stream name (ignored - path already contains full depot path) repo=self.repo, code_mapping=None, # type: ignore[arg-type] + revision=None, ) blames = self.p4_client.get_blame_for_files([file], extra={}) @@ -207,6 +211,7 @@ def test_get_blame_for_files_p4_exception(self, mock_logger, mock_p4_class): ref="", repo=self.repo, code_mapping=None, # type: ignore[arg-type] + revision=None, ) file2 = SourceLineInfo( path="src/missing.py", @@ -214,6 +219,7 @@ def test_get_blame_for_files_p4_exception(self, mock_logger, mock_p4_class): ref="", repo=self.repo, code_mapping=None, # type: ignore[arg-type] + revision=None, ) blames = self.p4_client.get_blame_for_files([file1, file2], extra={}) @@ -253,6 +259,7 @@ def test_get_blame_for_files_invalid_time(self, mock_logger, mock_p4_class): ref="", repo=self.repo, code_mapping=None, # type: ignore[arg-type] + revision=None, ) blames = self.p4_client.get_blame_for_files([file], extra={}) @@ -609,3 +616,63 @@ def test_build_depot_path_nested_depot(self): path = self.p4_client.build_depot_path(nested_repo, "src/main.cpp") assert path == "//depot/game/project/src/main.cpp" + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_blame_for_files_with_revision(self, mock_p4_class): + """Test get_blame_for_files uses revision from frame to get exact changelist""" + # Mock P4 instance + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock p4 changes response for file with specific revision + mock_p4.run.side_effect = [ + # First call: changes for file with revision + [ + { + "change": "12345", + "user": "johndoe", + "time": "1609459200", # 2021-01-01 00:00:00 UTC + "desc": "Fix bug in main module", + } + ], + # Second call: user lookup for johndoe + [ + { + "User": "johndoe", + "Email": "johndoe@example.com", + "FullName": "John Doe", + "Update": "2021/01/01 00:00:00", + } + ], + ] + + file_with_revision = SourceLineInfo( + path="src/main.cpp", + lineno=42, + ref="main", + repo=self.repo, + code_mapping=None, # type: ignore[arg-type] + revision="12345", # Revision from symcache + ) + + blames = self.p4_client.get_blame_for_files([file_with_revision], extra={}) + + assert len(blames) == 1 + + # Verify the blame info + assert blames[0].path == "src/main.cpp" + assert blames[0].lineno == 42 + assert blames[0].commit.commitId == "12345" + assert blames[0].commit.commitAuthorName == "John Doe" + assert blames[0].commit.commitAuthorEmail == "johndoe@example.com" + assert blames[0].commit.commitMessage == "Fix bug in main module" + + # Verify that p4 changes was called with the revision appended to depot path + # First call should be: p4.run("changes", "-m", "1", "-l", "//depot/src/main.cpp#12345") + assert mock_p4.run.call_count == 2 + first_call_args = mock_p4.run.call_args_list[0][0] + assert first_call_args[0] == "changes" + assert first_call_args[1] == "-m" + assert first_call_args[2] == "1" + assert first_call_args[3] == "-l" + assert first_call_args[4] == "//depot/src/main.cpp#12345" diff --git a/tests/sentry/integrations/perforce/test_stacktrace_link.py b/tests/sentry/integrations/perforce/test_stacktrace_link.py index 11ecfb5155bee4..008a29fc39bbb5 100644 --- a/tests/sentry/integrations/perforce/test_stacktrace_link.py +++ b/tests/sentry/integrations/perforce/test_stacktrace_link.py @@ -109,6 +109,33 @@ def test_get_stacktrace_config_cpp_path_with_revision(self, mock_check_file): assert result["error"] is None assert result["src_path"] == "game/src/main.cpp#1" + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_get_stacktrace_config_with_revision_field(self, mock_check_file): + """Test stacktrace link generation using separate revision field from symcache""" + mock_check_file.return_value = {"depotFile": "//depot/game/src/main.cpp"} + ctx: StacktraceLinkContext = { + "file": "depot/game/src/main.cpp", + "filename": "depot/game/src/main.cpp", + "abs_path": "depot/game/src/main.cpp", + "platform": "native", + "sdk_name": "sentry.native", + "commit_id": None, + "group_id": None, + "line_no": None, + "module": None, + "package": None, + "revision": "12345", # Revision from symcache as separate field + } + + result = get_stacktrace_config([self.code_mapping], ctx) + + assert result["source_url"] is not None + assert isinstance(result["source_url"], str) + # Verify revision is included in URL + assert "depot/game/src/main.cpp" in result["source_url"] + assert "#12345" in result["source_url"] or "12345" in result["source_url"] + assert result["error"] is None + def test_get_stacktrace_config_no_matching_code_mapping(self): """Test stacktrace link when no code mapping matches""" ctx: StacktraceLinkContext = { diff --git a/tests/sentry/integrations/source_code_management/test_commit_context.py b/tests/sentry/integrations/source_code_management/test_commit_context.py index 5d7c655c016ae0..fc27ac21dcaa6d 100644 --- a/tests/sentry/integrations/source_code_management/test_commit_context.py +++ b/tests/sentry/integrations/source_code_management/test_commit_context.py @@ -43,7 +43,12 @@ def setUp(self) -> None: name="example/repo", ) self.source_line = SourceLineInfo( - lineno=10, path="src/file.py", ref="main", repo=self.repo, code_mapping=Mock() + lineno=10, + path="src/file.py", + ref="main", + repo=self.repo, + code_mapping=Mock(), + revision=None, ) @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") diff --git a/tests/sentry/lang/native/test_processing.py b/tests/sentry/lang/native/test_processing.py index 01e70542b486f3..b80748512bd309 100644 --- a/tests/sentry/lang/native/test_processing.py +++ b/tests/sentry/lang/native/test_processing.py @@ -13,6 +13,7 @@ from sentry.lang.native.processing import ( ELECTRON_FIRST_MODULE_REWRITE_RULES, + _merge_frame, _merge_image, get_frames_for_symbolication, process_native_stacktraces, @@ -415,3 +416,44 @@ def test_il2cpp_symbolication(mock_symbolicator, default_project) -> None: == "/Users/swatinem/Coding/sentry-unity/samples/unity-of-bugs/Assets/Scripts/BugFarmButtons.cs" ) assert frame["lineno"] == 51 + + +def test_merge_frame_with_revision() -> None: + """Test that _merge_frame correctly extracts and merges the revision field from symbolicator.""" + new_frame: dict[str, Any] = {} + symbolicated = { + "function": "main", + "abs_path": "//depot/main/src/file.cpp", + "filename": "file.cpp", + "lineno": 42, + "revision": "12345", + "source_link": "https://perforce.example.com/file.cpp#12345", + } + + _merge_frame(new_frame, symbolicated, platform="native") + + assert new_frame["function"] == "main" + assert new_frame["abs_path"] == "//depot/main/src/file.cpp" + assert new_frame["filename"] == "file.cpp" + assert new_frame["lineno"] == 42 + assert new_frame["revision"] == "12345" + assert new_frame["source_link"] == "https://perforce.example.com/file.cpp#12345" + + +def test_merge_frame_without_revision() -> None: + """Test that _merge_frame works correctly when revision field is not present.""" + new_frame: dict[str, Any] = {} + symbolicated = { + "function": "main", + "abs_path": "/path/to/file.cpp", + "filename": "file.cpp", + "lineno": 42, + } + + _merge_frame(new_frame, symbolicated, platform="native") + + assert new_frame["function"] == "main" + assert new_frame["abs_path"] == "/path/to/file.cpp" + assert new_frame["filename"] == "file.cpp" + assert new_frame["lineno"] == 42 + assert "revision" not in new_frame diff --git a/tests/sentry/tasks/test_commit_context.py b/tests/sentry/tasks/test_commit_context.py index 0b73a6982a038b..af29dfc449b8a7 100644 --- a/tests/sentry/tasks/test_commit_context.py +++ b/tests/sentry/tasks/test_commit_context.py @@ -113,6 +113,7 @@ def setUp(self) -> None: path="sentry/recent.py", ref="master", code_mapping=self.code_mapping, + revision=None, lineno=30, commit=CommitInfo( commitId="commit-id-recent", @@ -127,6 +128,7 @@ def setUp(self) -> None: path="sentry/recent.py", ref="master", code_mapping=self.code_mapping, + revision=None, lineno=30, commit=CommitInfo( commitId="commit-id-old", @@ -141,6 +143,7 @@ def setUp(self) -> None: path="sentry/models/release.py", ref="master", code_mapping=self.code_mapping, + revision=None, lineno=39, commit=CommitInfo( commitId="existing-commit", @@ -155,6 +158,7 @@ def setUp(self) -> None: path="sentry/not_existing.py", ref="master", code_mapping=self.code_mapping, + revision=None, lineno=40, commit=CommitInfo( commitId="commit-id", @@ -413,6 +417,7 @@ def test_success_external_author_no_user(self, mock_get_commit_context: MagicMoc path="sentry/external.py", ref="master", code_mapping=self.code_mapping, + revision=None, lineno=50, commit=CommitInfo( commitId="external-commit-id", @@ -561,6 +566,7 @@ def test_maps_correct_files( ref="master", repo=code_mapping_defined_stack_root.repository, code_mapping=code_mapping_defined_stack_root, + revision=None, ) ], extra={ @@ -952,6 +958,7 @@ def test_filters_invalid_and_dedupes_frames( ref="master", repo=self.repo, code_mapping=self.code_mapping, + revision=None, ), ], extra={ @@ -1015,6 +1022,7 @@ def setUp(self) -> None: path="sentry/models/release.py", ref="master", code_mapping=self.code_mapping, + revision=None, lineno=39, commit=CommitInfo( commitId="asdfwreqr", diff --git a/tests/sentry/tasks/test_post_process.py b/tests/sentry/tasks/test_post_process.py index 43047b4d9d20bb..90ab6e9b624b70 100644 --- a/tests/sentry/tasks/test_post_process.py +++ b/tests/sentry/tasks/test_post_process.py @@ -1581,6 +1581,7 @@ def setUp(self) -> None: path="sentry/models/release.py", ref="master", repo=self.repo, + revision=None, commit=CommitInfo( commitId="asdfwreqr", committedDate=(timezone.now() - timedelta(days=2)),