feat(perforce): Add missing perforce test#116006
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| resp = self.client.post( | ||
| url, | ||
| data={ | ||
| "p4port": "ssl:perforce.example.com:1666", | ||
| "user": "sentry-bot", | ||
| "authType": "password", | ||
| "password": "secret", | ||
| }, | ||
| format="json", | ||
| ) |
There was a problem hiding this comment.
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.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit eb01b69. Configure here.
| "password": "secret", | ||
| }, | ||
| format="json", | ||
| ) |
There was a problem hiding this comment.
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)
Reviewed by Cursor Bugbot for commit eb01b69. Configure here.
| def test_step_data_is_empty(self) -> None: | ||
| step = PerforceInstallationApiStep() | ||
| data = step.get_step_data(None, None) # type: ignore[arg-type] | ||
| assert data == {} |
There was a problem hiding this comment.
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.
Reviewed by Cursor Bugbot for commit eb01b69. Configure here.
Backend Test FailuresFailures on
|
|
This was hanging around in my commits, it probably needs updated now |
1 participant
No description provided.