diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index f4961ed2aaa935..2af04bd3bb4eeb 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -3,6 +3,7 @@ import logging from collections.abc import Generator, Sequence from contextlib import contextmanager +from datetime import datetime, timezone from typing import Any, TypedDict from P4 import P4, P4Exception @@ -12,6 +13,7 @@ from sentry.integrations.services.integration import RpcIntegration, RpcOrganizationIntegration from sentry.integrations.source_code_management.commit_context import ( CommitContextClient, + CommitInfo, FileBlameInfo, SourceLineInfo, ) @@ -165,7 +167,7 @@ def _connect(self) -> Generator[P4]: # Assert SSL trust after connection (if needed) # This must be done after p4.connect() but before p4.run_login() - if self.ssl_fingerprint and self.p4port.startswith("ssl:"): + if self.ssl_fingerprint and self.p4port.startswith("ssl"): try: p4.run_trust("-i", self.ssl_fingerprint) except P4Exception as trust_error: @@ -357,6 +359,46 @@ def get_user(self, username: str) -> P4UserInfo | None: # User not found - return None (not an error condition) return None + def get_author_info_from_cache( + self, username: str, user_cache: dict[str, P4UserInfo | None] + ) -> tuple[str, str]: + """ + Get author email and name from username with caching. + + Args: + username: Perforce username + user_cache: Cache dictionary for user lookups + + Returns: + Tuple of (author_email, author_name) + """ + author_email = f"{username}@perforce" + author_name = username + + # Fetch user info if not in cache + if username not in user_cache: + try: + user_cache[username] = self.get_user(username) + except Exception as e: + logger.warning( + "perforce.get_author_info.user_lookup_failed", + extra={ + "username": username, + "error": str(e), + "error_type": type(e).__name__, + }, + ) + user_cache[username] = None + + user_info = user_cache.get(username) + if user_info: + if user_info.get("email"): + author_email = user_info["email"] + if user_info.get("full_name"): + author_name = user_info["full_name"] + + return author_email, author_name + def get_changes( self, depot_path: str, @@ -435,17 +477,102 @@ def get_blame_for_files( self, files: Sequence[SourceLineInfo], extra: dict[str, Any] ) -> list[FileBlameInfo]: """ - Get blame information for multiple files using p4 filelog. + Get blame information for multiple files using p4 changes. - Uses 'p4 filelog' + 'p4 describe' which is much faster than 'p4 annotate'. - Returns the most recent changelist that modified each file. + Uses 'p4 changes -m 1 -l' to get the most recent changelist that modified each file. + This is simpler and faster than using p4 filelog + p4 describe. Note: This does not provide line-specific blame. It returns the most recent changelist for the entire file, which is sufficient for suspect commit detection. + API docs: + - p4 changes: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_changes.html + Returns a list of FileBlameInfo objects containing commit details for each file. + + Performance notes: + - Makes ~2 P4 API calls per file: changes (with -l for description), user (cached) + - User lookups are cached within the request to minimize redundant calls + - Perforce doesn't have explicit rate limiting like GitHub + - Individual file failures are caught and logged without failing entire batch """ - return [] + blames: list[FileBlameInfo] = [] + user_cache: dict[str, P4UserInfo | None] = {} + + with self._connect() as p4: + for file in files: + try: + # Build depot path for the file (includes stream if specified) + # file.ref contains the stream but we are ignoring it since it's + # already part of the depot path we get from stacktrace (SourceLineInfo) + depot_path = self.build_depot_path(file.repo, file.path, None) + + # 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 + changes = p4.run("changes", "-m", "1", "-l", depot_path) + + if changes and len(changes) > 0: + change = changes[0] + changelist = change.get("change", "") + username = change.get("user", "unknown") + + # Get author email and name with caching + author_email, author_name = self.get_author_info_from_cache( + username, user_cache + ) + + # Handle potentially null/invalid time field + time_value = change.get("time") or 0 + try: + time_int = int(time_value) + except (TypeError, ValueError) as e: + logger.warning( + "perforce.client.get_blame_for_files.invalid_time_value", + extra={ + **extra, + "changelist": changelist, + "time_value": time_value, + "error": str(e), + "repo_name": file.repo.name, + "file_path": file.path, + }, + ) + time_int = 0 + + commit = CommitInfo( + commitId=str(changelist), + committedDate=datetime.fromtimestamp(time_int, tz=timezone.utc), + commitMessage=change.get("desc", "").strip(), + commitAuthorName=author_name, + commitAuthorEmail=author_email, + ) + + blame_info = FileBlameInfo( + lineno=file.lineno, + path=file.path, + ref=file.ref, + repo=file.repo, + code_mapping=file.code_mapping, + commit=commit, + ) + blames.append(blame_info) + + except P4Exception as e: + # Log but don't fail for individual file errors + logger.warning( + "perforce.client.get_blame_for_files.error", + extra={ + **extra, + "error": str(e), + "repo_name": file.repo.name, + "file_path": file.path, + "file_lineno": file.lineno, + }, + ) + continue + + return blames def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index bf034e292574bf..3262275bf5e5d2 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -45,7 +45,7 @@ class PerforceMetadata(TypedDict, total=False): DESCRIPTION = """ -Connect your Sentry organization to your Perforce/Helix Core server to enable +Connect your Sentry organization to your P4 Core server to enable stacktrace linking, commit tracking, suspect commit detection, and code ownership. View source code directly from error stack traces and identify suspect commits that may have introduced issues. @@ -55,7 +55,7 @@ class PerforceMetadata(TypedDict, total=False): FeatureDescription( """ Link your Sentry stack traces back to your Perforce depot files with support - for Helix Swarm web viewer. Automatically maps error locations to + for P4 Code Review viewer. Automatically maps error locations to source code using configurable code mappings. """, IntegrationFeatures.STACKTRACE_LINK, @@ -145,8 +145,8 @@ class PerforceInstallationForm(forms.Form): required=False, ) web_url = forms.URLField( - label=_("Helix Swarm URL (Optional)"), - help_text=_("Optional: URL to Helix Swarm web viewer for browsing files"), + label=_("P4 Code Review URL (Optional)"), + help_text=_("Optional: URL to P4 Code Review web viewer for browsing files"), widget=forms.URLInput(attrs={"placeholder": "https://swarm.company.com"}), required=False, assume_scheme="https", @@ -166,7 +166,7 @@ def clean_web_url(self) -> str: class PerforceIntegration(RepositoryIntegration, CommitContextIntegration): """ - Integration for Perforce/Helix Core version control system. + Integration for P4 Core version control system. Provides stacktrace linking to depot files and suspect commit detection. """ @@ -471,9 +471,9 @@ def get_organization_config(self) -> list[dict[str, Any]]: { "name": "web_url", "type": "string", - "label": "Helix Swarm URL (Optional)", + "label": "P4 Core URL (Optional)", "placeholder": "https://swarm.company.com", - "help": "Optional: URL to Helix Swarm web viewer for browsing files", + "help": "Optional: URL to P4 Core web viewer for browsing files", "required": False, }, ] diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index f2235b4649f8b4..1e27ac59d7a75a 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -164,37 +164,9 @@ def _extract_commit_info( # Convert Unix timestamp to ISO 8601 format timestamp = datetime.fromtimestamp(time_int, tz=timezone.utc).isoformat() - # Get user information from Perforce + # Get user information from Perforce using shared helper username = change.get("user", "unknown") - author_email = f"{username}@perforce" - author_name = username - - # Fetch user info if not in cache (skip "unknown" placeholder) - if username != "unknown" and username not in user_cache: - try: - user_cache[username] = client.get_user(username) - except Exception as e: - # Log user lookup failures but don't fail the entire commit processing - logger.warning( - "perforce.format_commits.user_lookup_failed", - extra={ - "changelist": change.get("change"), - "username": username, - "error": str(e), - "error_type": type(e).__name__, - }, - ) - # Cache None to avoid repeated failed lookups for the same user - user_cache[username] = None - - user_info = user_cache.get(username) - if user_info: - # Use actual email from Perforce if available - if user_info.get("email"): - author_email = user_info["email"] - # Use full name from Perforce if available - if user_info.get("full_name"): - author_name = user_info["full_name"] + author_email, author_name = client.get_author_info_from_cache(username, user_cache) return P4CommitInfo( id=str(change["change"]), diff --git a/src/sentry/templates/sentry/integrations/perforce-config.html b/src/sentry/templates/sentry/integrations/perforce-config.html index e7288bbe251c98..df2a500937a193 100644 --- a/src/sentry/templates/sentry/integrations/perforce-config.html +++ b/src/sentry/templates/sentry/integrations/perforce-config.html @@ -19,8 +19,8 @@ {% block title %} {% trans "Perforce Setup" %} | {{ block.super }} {% endblock %} {% block main %} -

{% trans "Configure Perforce/Helix Core Connection" %}

-

{% trans "Enter your Perforce server credentials to connect Sentry with your Perforce/Helix Core server." %}

+

{% trans "Configure P4 Core Connection" %}

+

{% trans "Enter your Perforce server credentials to connect Sentry with your P4 Core server." %}

{% trans "See the" %} {% trans "docs" %} {% trans "for more information." %}

diff --git a/tests/sentry/integrations/perforce/test_client.py b/tests/sentry/integrations/perforce/test_client.py new file mode 100644 index 00000000000000..a2152984bb25e1 --- /dev/null +++ b/tests/sentry/integrations/perforce/test_client.py @@ -0,0 +1,611 @@ +from datetime import datetime, timezone +from unittest import mock + +import pytest + +from sentry.integrations.perforce.client import PerforceClient +from sentry.integrations.source_code_management.commit_context import SourceLineInfo +from sentry.models.repository import Repository +from sentry.testutils.cases import TestCase + + +class PerforceClientTest(TestCase): + def setUp(self): + super().setUp() + self.integration = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce Test", + external_id="perforce-test", + metadata={ + "p4port": "perforce.example.com:1666", + "user": "testuser", + "password": "testpass", + "auth_type": "password", + }, + ) + self.org_integration = self.integration.organizationintegration_set.first() + + self.repo = Repository.objects.create( + organization_id=self.organization.id, + name="//depot", + provider="integrations:perforce", + external_id="//depot", + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + self.p4_client = PerforceClient( + integration=self.integration, org_integration=self.org_integration + ) + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_blame_for_files_success(self, mock_p4_class): + """Test get_blame_for_files returns commit info for files""" + # Mock P4 instance + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock p4 changes responses (simpler now - one call per file) + mock_p4.run.side_effect = [ + # First call: changes for file 1 + [ + { + "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": "", "FullName": ""}], + # Third call: changes for file 2 + [ + { + "change": "12346", + "user": "janedoe", + "time": "1609545600", # 2021-01-02 00:00:00 UTC + "desc": "Add utility functions", + } + ], + # Fourth call: user lookup for janedoe + [{"User": "janedoe", "Email": "", "FullName": ""}], + ] + + file1 = SourceLineInfo( + path="src/main.py", + lineno=10, + ref="", + repo=self.repo, + code_mapping=None, # type: ignore[arg-type] + ) + file2 = SourceLineInfo( + path="src/utils.py", + lineno=25, + ref="", + repo=self.repo, + code_mapping=None, # type: ignore[arg-type] + ) + + blames = self.p4_client.get_blame_for_files([file1, file2], extra={}) + + assert len(blames) == 2 + + # Check first blame + assert blames[0].path == "src/main.py" + assert blames[0].lineno == 10 + assert blames[0].commit.commitId == "12345" + assert blames[0].commit.commitAuthorName == "johndoe" + assert blames[0].commit.commitAuthorEmail == "johndoe@perforce" + assert blames[0].commit.commitMessage == "Fix bug in main module" + assert blames[0].commit.committedDate == datetime(2021, 1, 1, 0, 0, 0, tzinfo=timezone.utc) + + # Check second blame + assert blames[1].path == "src/utils.py" + assert blames[1].lineno == 25 + assert blames[1].commit.commitId == "12346" + assert blames[1].commit.commitAuthorName == "janedoe" + assert blames[1].commit.commitAuthorEmail == "janedoe@perforce" + assert blames[1].commit.commitMessage == "Add utility functions" + assert blames[1].commit.committedDate == datetime(2021, 1, 2, 0, 0, 0, tzinfo=timezone.utc) + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_blame_for_files_no_changelist(self, mock_p4_class): + """Test get_blame_for_files handles files with no changelist""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock p4 changes response with empty result (no changes for this file) + mock_p4.run.return_value = [] + + file = SourceLineInfo( + path="src/new_file.py", + lineno=10, + ref="", + repo=self.repo, + code_mapping=None, # type: ignore[arg-type] + ) + + blames = self.p4_client.get_blame_for_files([file], extra={}) + + # Should return empty list when no changelist found + assert len(blames) == 0 + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_blame_for_files_with_stream(self, mock_p4_class): + """Test get_blame_for_files handles files with stream (ref). + + Note: The stream name in ref is ignored because the path from SourceLineInfo + already contains the full depot path including any stream/branch information. + """ + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + mock_p4.run.side_effect = [ + # changes for file with stream + [ + { + "change": "12345", + "user": "johndoe", + "time": "1609459200", + "desc": "Initial commit", + } + ], + # user lookup for johndoe + [{"User": "johndoe", "Email": "", "FullName": ""}], + ] + + file = SourceLineInfo( + path="src/main.py", + lineno=10, + ref="main", # Stream name (ignored - path already contains full depot path) + repo=self.repo, + code_mapping=None, # type: ignore[arg-type] + ) + + blames = self.p4_client.get_blame_for_files([file], extra={}) + + assert len(blames) == 1 + # Verify changes was called with the depot path (stream from ref is not used) + assert mock_p4.run.call_args_list[0][0] == ( + "changes", + "-m", + "1", + "-l", + "//depot/src/main.py", + ) + + @mock.patch("sentry.integrations.perforce.client.P4") + @mock.patch("sentry.integrations.perforce.client.logger") + def test_get_blame_for_files_p4_exception(self, mock_logger, mock_p4_class): + """Test get_blame_for_files handles P4 exceptions gracefully""" + from P4 import P4Exception + + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # First file succeeds, second file throws exception + mock_p4.run.side_effect = [ + # File 1 changes + [ + { + "change": "12345", + "user": "johndoe", + "time": "1609459200", + "desc": "Initial commit", + } + ], + # File 1 user lookup for johndoe + [{"User": "johndoe", "Email": "", "FullName": ""}], + # File 2 changes - throws exception + P4Exception("File not found"), + ] + + file1 = SourceLineInfo( + path="src/main.py", + lineno=10, + ref="", + repo=self.repo, + code_mapping=None, # type: ignore[arg-type] + ) + file2 = SourceLineInfo( + path="src/missing.py", + lineno=20, + ref="", + repo=self.repo, + code_mapping=None, # type: ignore[arg-type] + ) + + blames = self.p4_client.get_blame_for_files([file1, file2], extra={}) + + # Should return blame for file1 only + assert len(blames) == 1 + assert blames[0].path == "src/main.py" + + # Should log warning for file2 + assert mock_logger.warning.called + + @mock.patch("sentry.integrations.perforce.client.P4") + @mock.patch("sentry.integrations.perforce.client.logger") + def test_get_blame_for_files_invalid_time(self, mock_logger, mock_p4_class): + """Test get_blame_for_files handles invalid time values gracefully""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock with invalid time value (non-numeric string) + mock_p4.run.side_effect = [ + # changes with invalid time value + [ + { + "change": "12345", + "user": "johndoe", + "time": "invalid", # Invalid time value that will raise ValueError + "desc": "Initial commit", + } + ], + # user lookup for johndoe + [{"User": "johndoe", "Email": "", "FullName": ""}], + ] + + file = SourceLineInfo( + path="src/main.py", + lineno=10, + ref="", + repo=self.repo, + code_mapping=None, # type: ignore[arg-type] + ) + + blames = self.p4_client.get_blame_for_files([file], extra={}) + + # Should still return blame with epoch time (0) + assert len(blames) == 1 + assert blames[0].commit.committedDate == datetime(1970, 1, 1, 0, 0, 0, tzinfo=timezone.utc) + + # Should log warning about invalid time + assert mock_logger.warning.called + warning_calls = [str(call) for call in mock_logger.warning.call_args_list] + assert any("invalid_time_value" in call for call in warning_calls) + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_depots(self, mock_p4_class): + """Test get_depots returns depot list""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock depots response + mock_p4.run.return_value = [ + {"name": "depot", "type": "local", "desc": "Main depot"}, + {"name": "myproject", "type": "stream", "desc": "Project stream depot"}, + ] + + depots = self.p4_client.get_depots() + + # Verify p4 depots command was called + mock_p4.run.assert_called_once_with("depots") + + # Check results + assert len(depots) == 2 + assert depots[0]["name"] == "depot" + assert depots[0]["type"] == "local" + assert depots[0]["description"] == "Main depot" + + assert depots[1]["name"] == "myproject" + assert depots[1]["type"] == "stream" + assert depots[1]["description"] == "Project stream depot" + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_user_existing(self, mock_p4_class): + """Test get_user returns user info for existing user""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock user response for existing user + mock_p4.run.return_value = [ + { + "User": "johndoe", + "Email": "john.doe@example.com", + "FullName": "John Doe", + "Update": "2021/01/01 12:00:00", # Indicates user exists + } + ] + + user_info = self.p4_client.get_user("johndoe") + + # Verify p4 user command was called + mock_p4.run.assert_called_once_with("user", "-o", "johndoe") + + # Check results + assert user_info is not None + assert user_info["username"] == "johndoe" + assert user_info["email"] == "john.doe@example.com" + assert user_info["full_name"] == "John Doe" + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_user_nonexistent(self, mock_p4_class): + """Test get_user returns None for non-existent user""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock user response for non-existent user (template with no Update field) + mock_p4.run.return_value = [ + { + "User": "newuser", + "Email": "", + "FullName": "", + # No Update field - indicates user doesn't exist + } + ] + + user_info = self.p4_client.get_user("newuser") + + # Should return None for non-existent user + assert user_info is None + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_changes_without_range(self, mock_p4_class): + """Test get_changes returns changelists without start/end range""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock changes response + mock_p4.run.return_value = [ + { + "change": "12346", + "user": "janedoe", + "client": "jane-workspace", + "time": "1609545600", + "desc": "Add utility functions", + }, + { + "change": "12345", + "user": "johndoe", + "client": "john-workspace", + "time": "1609459200", + "desc": "Fix bug in main module", + }, + ] + + changes = self.p4_client.get_changes("//depot/...", max_changes=20) + + # Verify p4 changes command was called with correct args + mock_p4.run.assert_called_once_with("changes", "-m", "20", "-l", "//depot/...") + + # Check results + assert len(changes) == 2 + assert changes[0]["change"] == "12346" + assert changes[0]["user"] == "janedoe" + assert changes[1]["change"] == "12345" + assert changes[1]["user"] == "johndoe" + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_changes_with_end_cl(self, mock_p4_class): + """Test get_changes filters by end changelist""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + mock_p4.run.return_value = [ + { + "change": "12346", + "user": "janedoe", + "client": "jane-ws", + "time": "1609545600", + "desc": "Add utils", + }, + { + "change": "12345", + "user": "johndoe", + "client": "john-ws", + "time": "1609459200", + "desc": "Fix bug", + }, + ] + + changes = self.p4_client.get_changes("//depot/...", max_changes=20, end_cl=12346) + + # Verify -e flag was used for end_cl + mock_p4.run.assert_called_once_with( + "changes", "-m", "20", "-l", "-e", "12346", "//depot/..." + ) + + assert len(changes) == 2 + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_changes_with_start_cl(self, mock_p4_class): + """Test get_changes filters by start changelist (exclusive)""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock returns changes 12345, 12346, 12347 + mock_p4.run.return_value = [ + { + "change": "12347", + "user": "user3", + "client": "ws3", + "time": "1609632000", + "desc": "Change 3", + }, + { + "change": "12346", + "user": "user2", + "client": "ws2", + "time": "1609545600", + "desc": "Change 2", + }, + { + "change": "12345", + "user": "user1", + "client": "ws1", + "time": "1609459200", + "desc": "Change 1", + }, + ] + + # Filter out changes <= 12345 (only want changes > 12345) + changes = self.p4_client.get_changes("//depot/...", max_changes=20, start_cl=12345) + + # Verify no -s flag (Perforce doesn't have one) + # Client-side filtering is done for start_cl + assert len(changes) == 2 + assert changes[0]["change"] == "12347" + assert changes[1]["change"] == "12346" + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_changes_with_range(self, mock_p4_class): + """Test get_changes filters by start and end changelist range""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + mock_p4.run.return_value = [ + { + "change": "12347", + "user": "user3", + "client": "ws3", + "time": "1609632000", + "desc": "Change 3", + }, + { + "change": "12346", + "user": "user2", + "client": "ws2", + "time": "1609545600", + "desc": "Change 2", + }, + { + "change": "12345", + "user": "user1", + "client": "ws1", + "time": "1609459200", + "desc": "Change 1", + }, + ] + + # Get changes in range (12345, 12348] = 12346, 12347 + changes = self.p4_client.get_changes( + "//depot/...", max_changes=20, start_cl=12345, end_cl=12348 + ) + + # Should filter out 12345 (exclusive lower bound) + assert len(changes) == 2 + assert changes[0]["change"] == "12347" + assert changes[1]["change"] == "12346" + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_changes_type_validation(self, mock_p4_class): + """Test get_changes validates that start_cl and end_cl are integers""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Test with string start_cl + with pytest.raises(TypeError, match="start_cl must be an integer"): + self.p4_client.get_changes("//depot/...", start_cl="12345") # type: ignore[arg-type] + + # Test with string end_cl + with pytest.raises(TypeError, match="end_cl must be an integer"): + self.p4_client.get_changes("//depot/...", end_cl="12346") # type: ignore[arg-type] + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_check_file_exists(self, mock_p4_class): + """Test check_file returns file info when file exists""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock files response + mock_p4.run.return_value = [ + { + "depotFile": "//depot/src/main.py", + "rev": "5", + "change": "12345", + "action": "edit", + } + ] + + result = self.p4_client.check_file(self.repo, "src/main.py", None) + + # Verify p4 files command was called + mock_p4.run.assert_called_once_with("files", "//depot/src/main.py") + + # Check result + assert result is not None + assert isinstance(result, dict) + assert result["depotFile"] == "//depot/src/main.py" + assert result["rev"] == "5" + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_check_file_not_exists(self, mock_p4_class): + """Test check_file returns None when file doesn't exist""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock P4Exception for non-existent file + from P4 import P4Exception + + mock_p4.run.side_effect = P4Exception("File not found") + + result = self.p4_client.check_file(self.repo, "src/missing.py", None) + + # Should return None for missing file + assert result is None + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_check_file_empty_result(self, mock_p4_class): + """Test check_file returns None when result has no depotFile""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock empty result (just warnings) + mock_p4.run.return_value = [{"warning": "File is not in depot"}] + + result = self.p4_client.check_file(self.repo, "src/file.py", None) + + # Should return None when no depotFile in result + assert result is None + + def test_build_depot_path_relative(self): + """Test build_depot_path with relative path""" + # Test with simple relative path + path = self.p4_client.build_depot_path(self.repo, "app/services/processor.py") + assert path == "//depot/app/services/processor.py" + + # Test with depot name in path (should strip it) + path = self.p4_client.build_depot_path(self.repo, "depot/app/services/processor.py") + assert path == "//depot/app/services/processor.py" + + def test_build_depot_path_absolute(self): + """Test build_depot_path with absolute path""" + path = self.p4_client.build_depot_path(self.repo, "//depot/app/services/processor.py") + assert path == "//depot/app/services/processor.py" + + def test_build_depot_path_with_stream(self): + """Test build_depot_path with stream parameter""" + path = self.p4_client.build_depot_path( + self.repo, "app/services/processor.py", stream="main" + ) + assert path == "//depot/main/app/services/processor.py" + + def test_build_depot_path_with_revision(self): + """Test build_depot_path preserves #revision syntax""" + # Test with relative path + path = self.p4_client.build_depot_path(self.repo, "app/services/processor.py#42") + assert path == "//depot/app/services/processor.py#42" + + # Test with absolute path + path = self.p4_client.build_depot_path(self.repo, "//depot/app/main.cpp#1") + assert path == "//depot/app/main.cpp#1" + + # Test with stream and revision + path = self.p4_client.build_depot_path(self.repo, "app/main.cpp#5", stream="dev") + assert path == "//depot/dev/app/main.cpp#5" + + def test_build_depot_path_nested_depot(self): + """Test build_depot_path with nested depot paths""" + nested_repo = Repository.objects.create( + organization_id=self.organization.id, + name="//depot/game/project", + provider="integrations:perforce", + external_id="//depot/game/project", + integration_id=self.integration.id, + config={"depot_path": "//depot/game/project"}, + ) + + path = self.p4_client.build_depot_path(nested_repo, "src/main.cpp") + assert path == "//depot/game/project/src/main.cpp" diff --git a/tests/sentry/integrations/perforce/test_stacktrace_link.py b/tests/sentry/integrations/perforce/test_stacktrace_link.py index 715c19ae50b47e..11ecfb5155bee4 100644 --- a/tests/sentry/integrations/perforce/test_stacktrace_link.py +++ b/tests/sentry/integrations/perforce/test_stacktrace_link.py @@ -56,6 +56,12 @@ def tearDown(self): def test_get_stacktrace_config_python_path(self): """Test stacktrace link generation for Python SDK path""" + self.check_file_patcher.stop() + self.check_file_patcher = patch( + "sentry.integrations.perforce.client.PerforceClient.check_file", + return_value={"depotFile": "//depot/app/services/processor.py"}, + ) + self.check_file_patcher.start() ctx: StacktraceLinkContext = { "file": "depot/app/services/processor.py", "filename": "depot/app/services/processor.py", @@ -77,12 +83,14 @@ def test_get_stacktrace_config_python_path(self): assert result["error"] is None assert result["src_path"] == "app/services/processor.py" - def test_get_stacktrace_config_cpp_path_with_revision(self): - """Test stacktrace link generation for C++ path with @revision""" + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_get_stacktrace_config_cpp_path_with_revision(self, mock_check_file): + """Test stacktrace link generation for C++ path with #revision""" + mock_check_file.return_value = {"depotFile": "//depot/game/src/main.cpp"} ctx: StacktraceLinkContext = { - "file": "depot/game/src/main.cpp@42", - "filename": "depot/game/src/main.cpp@42", - "abs_path": "depot/game/src/main.cpp@42", + "file": "depot/game/src/main.cpp#1", + "filename": "depot/game/src/main.cpp#1", + "abs_path": "depot/game/src/main.cpp#1", "platform": "native", "sdk_name": "sentry.native", "commit_id": None, @@ -96,10 +104,10 @@ def test_get_stacktrace_config_cpp_path_with_revision(self): assert result["source_url"] is not None assert isinstance(result["source_url"], str) - # URL will be encoded: p4://depot/game/src/main.cpp%4042 + # URL will be encoded: p4://depot/game/src/main.cpp%231 assert "depot/game/src/main.cpp" in result["source_url"] assert result["error"] is None - assert result["src_path"] == "game/src/main.cpp@42" + assert result["src_path"] == "game/src/main.cpp#1" def test_get_stacktrace_config_no_matching_code_mapping(self): """Test stacktrace link when no code mapping matches""" @@ -122,8 +130,10 @@ def test_get_stacktrace_config_no_matching_code_mapping(self): assert result["error"] == "stack_root_mismatch" assert result["src_path"] is None - def test_get_stacktrace_config_multiple_code_mappings(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_get_stacktrace_config_multiple_code_mappings(self, mock_check_file): """Test stacktrace link with multiple code mappings""" + mock_check_file.return_value = {"depotFile": "//myproject/app/services/handler.py"} # Add another depot mapping myproject_repo = Repository.objects.create( name="//myproject", @@ -164,8 +174,10 @@ def test_get_stacktrace_config_multiple_code_mappings(self): assert "//myproject/app/services/handler.py" in result["source_url"] assert result["src_path"] == "app/services/handler.py" - def test_get_stacktrace_config_with_web_viewer(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_get_stacktrace_config_with_web_viewer(self, mock_check_file): """Test stacktrace link with P4Web viewer""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.py"} integration_with_web = self.create_integration( organization=self.organization, provider="perforce", @@ -223,8 +235,10 @@ def test_get_stacktrace_config_with_web_viewer(self): in result["source_url"] ) - def test_get_stacktrace_config_abs_path_fallback(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_get_stacktrace_config_abs_path_fallback(self, mock_check_file): """Test stacktrace link uses abs_path when filename is just basename""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.py"} ctx: StacktraceLinkContext = { "file": "processor.py", "filename": "processor.py", @@ -245,8 +259,10 @@ def test_get_stacktrace_config_abs_path_fallback(self): assert "//depot/app/services/processor.py" in result["source_url"] assert result["src_path"] == "app/services/processor.py" - def test_get_stacktrace_config_iteration_count(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_get_stacktrace_config_iteration_count(self, mock_check_file): """Test that iteration_count is incremented only for matching mappings""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.py"} # Add a non-matching mapping other_repo = Repository.objects.create( name="//other", @@ -285,8 +301,10 @@ def test_get_stacktrace_config_iteration_count(self): assert result["iteration_count"] == 1 assert result["source_url"] is not None - def test_get_stacktrace_config_stops_on_first_match(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_get_stacktrace_config_stops_on_first_match(self, mock_check_file): """Test that iteration stops after first successful match""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.py"} # Add another depot mapping (shouldn't be checked if first matches) # Use different project to avoid unique constraint project2 = self.create_project(organization=self.organization) @@ -360,6 +378,12 @@ def tearDown(self): def test_stacktrace_link_empty_stack_root(self): """Test stacktrace link with empty stack_root (shouldn't match anything)""" + self.check_file_patcher.stop() + self.check_file_patcher = patch( + "sentry.integrations.perforce.client.PerforceClient.check_file", + return_value={"depotFile": "//depot/app/services/processor.py"}, + ) + self.check_file_patcher.start() repo = Repository.objects.create( name="//depot", organization_id=self.organization.id, @@ -396,8 +420,10 @@ def test_stacktrace_link_empty_stack_root(self): # Empty stack_root should match any path assert result["source_url"] is not None - def test_stacktrace_link_with_special_characters_in_path(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_stacktrace_link_with_special_characters_in_path(self, mock_check_file): """Test stacktrace link with special characters in file path""" + mock_check_file.return_value = {"depotFile": "//depot/app/my services/processor-v2.py"} repo = Repository.objects.create( name="//depot", organization_id=self.organization.id, @@ -435,8 +461,10 @@ def test_stacktrace_link_with_special_characters_in_path(self): assert result["source_url"] is not None assert result["src_path"] == "app/my services/processor-v2.py" - def test_stacktrace_link_deeply_nested_path(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_stacktrace_link_deeply_nested_path(self, mock_check_file): """Test stacktrace link with very deeply nested path""" + mock_check_file.return_value = {"depotFile": "//depot/file.py"} repo = Repository.objects.create( name="//depot", organization_id=self.organization.id,