Skip to content

feat(perforce): Add missing perforce test #116006

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.

Sign up for GitHub

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

Draft
evanpurkhiser wants to merge 1 commit into
base: master
Choose a base branch
from
+130 −0
Draft

feat(perforce): Add missing perforce test #116006

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 130 additions & 0 deletions tests/sentry/integrations/perforce/test_pipeline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
from unittest.mock import patch

import responses
from django.urls import reverse

from sentry.integrations.perforce.integration import PerforceInstallationApiStep
from sentry.integrations.pipeline import IntegrationPipeline
from sentry.organizations.services.organization.serial import serialize_rpc_organization
from sentry.shared_integrations.exceptions import ApiError, ApiUnauthorized
from sentry.silo.base import SiloMode
from sentry.testutils.cases import APITestCase
from sentry.testutils.silo import assume_test_silo_mode, control_silo_test


@control_silo_test
class PerforceApiPipelineTest(APITestCase):
endpoint = "sentry-api-0-organization-pipeline"

def setUp(self):
super().setUp()
self.login_as(self.user)

def _get_pipeline_url(self) -> str:
return reverse(
self.endpoint,
args=[self.organization.slug, IntegrationPipeline.pipeline_name],
)

def _init_pipeline_in_session(self) -> IntegrationPipeline:
with assume_test_silo_mode(SiloMode.CELL):
rpc_org = serialize_rpc_organization(self.organization)

request = self.make_request(self.user)
pipeline = IntegrationPipeline(
request=request,
organization=rpc_org,
provider_key="perforce",
)
pipeline.initialize()
self.save_session()
return pipeline

@responses.activate
@patch(
"sentry.integrations.perforce.integration.PerforceClient.get_depots",
return_value=[{"name": "depot"}],
)
def test_successful_connection(self, mock_get_depots) -> None:
pipeline = self._init_pipeline_in_session()
pipeline.set_api_mode()
url = self._get_pipeline_url()

resp = self.client.get(url)
assert resp.status_code == 200
assert resp.data["step"] == "installation_config"
assert resp.data["data"] == {}

resp = self.client.post(
url,
data={
"p4port": "ssl:perforce.example.com:1666",
"user": "sentry-bot",
"authType": "password",
"password": "secret",
},
format="json",
)
Comment on lines +58 to +67
Copy link
Copy Markdown
Contributor

@sentry sentry Bot May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Tests for Perforce SSL connections are missing the required ssl_fingerprint field in the POST data, which will cause them to fail validation.
Severity: LOW

Suggested Fix

Update the POST data in the Perforce SSL tests (test_successful_connection, test_auth_failure_returns_error, test_connection_failure_returns_error) to include a valid ssl_fingerprint field alongside the p4port that uses an 'ssl:' prefix.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: tests/sentry/integrations/perforce/test_pipeline.py#L58-L67

Potential issue: The `PerforceInstallationSerializer` requires an `ssl_fingerprint` when
the `p4port` field starts with `'ssl'`. The newly added tests
(`test_successful_connection`, `test_auth_failure_returns_error`, and
`test_connection_failure_returns_error`) provide an SSL `p4port` but omit the required
`ssl_fingerprint`. This causes the serializer's `is_valid()` check to raise a
`ValidationError`, resulting in an HTTP 400 response. `test_successful_connection` will
fail its status code assertion, while the other tests will fail with a `KeyError` due to
an unexpected error response structure.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Copy Markdown
Contributor

@cursor cursor Bot May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests missing required sslFingerprint for SSL connections

Medium Severity

The test_successful_connection, test_auth_failure_returns_error, and test_connection_failure_returns_error tests all post a p4port starting with "ssl:" but omit sslFingerprint. The PerforceInstallationSerializer.validate method raises a ValidationError when p4port starts with "ssl" and ssl_fingerprint is falsy (its default is ""). This means all three tests will fail at serializer validation before reaching the mocked get_depots call. The equivalent tests in test_integration.py correctly include sslFingerprint in the POST data.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit eb01b69. Configure here.

assert resp.status_code == 200
assert resp.data["status"] == "complete"

@responses.activate
@patch(
"sentry.integrations.perforce.integration.PerforceClient.get_depots",
side_effect=ApiUnauthorized("bad credentials"),
)
def test_auth_failure_returns_error(self, mock_get_depots) -> None:
pipeline = self._init_pipeline_in_session()
pipeline.set_api_mode()
url = self._get_pipeline_url()

resp = self.client.post(
url,
data={
"p4port": "ssl:perforce.example.com:1666",
"user": "bad-user",
"authType": "password",
"password": "wrong",
},
format="json",
)
assert resp.status_code == 400
assert "Authentication failed" in resp.data["data"]["detail"]

@responses.activate
@patch(
"sentry.integrations.perforce.integration.PerforceClient.get_depots",
side_effect=ApiError("connection refused"),
)
def test_connection_failure_returns_error(self, mock_get_depots) -> None:
pipeline = self._init_pipeline_in_session()
pipeline.set_api_mode()
url = self._get_pipeline_url()

resp = self.client.post(
url,
data={
"p4port": "ssl:bad-host:1666",
"user": "user",
"authType": "password",
"password": "pass",
},
format="json",
)
assert resp.status_code == 400
assert "Failed to connect" in resp.data["data"]["detail"]

@responses.activate
def test_missing_required_fields(self) -> None:
pipeline = self._init_pipeline_in_session()
pipeline.set_api_mode()
url = self._get_pipeline_url()

resp = self.client.post(url, data={}, format="json")
assert resp.status_code == 400

@responses.activate
def test_step_data_is_empty(self) -> None:
step = PerforceInstallationApiStep()
data = step.get_step_data(None, None) # type: ignore[arg-type]
assert data == {}
Copy link
Copy Markdown
Contributor

@cursor cursor Bot May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate test class already exists in another file

Low Severity

The PerforceApiPipelineTest class with its helper methods and four of its five tests (test_successful_connection, test_auth_failure_returns_error, test_connection_failure_returns_error, test_missing_required_fields) is a near-exact copy of the identically named class already in test_integration.py (line 625). The only genuinely new test is test_step_data_is_empty. This duplication risks divergence (as already evidenced by the missing sslFingerprint bug) and increases maintenance burden. The new test could be added to the existing class instead.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit eb01b69. Configure here.

Loading