Skip to content

Commit 724008f

Browse files
author
Your Name
committed
fix: Address bot review feedback (issues 2, 3, 4)
- Add HTTP status checks to createPullRequest and getPullRequest - Add try/finally blocks to tests for proper cleanup on failures - Make error test assertions concrete (empty string or exception) - Improve testGetPullRequestWithInvalidNumber to use try/finally Note: Issue #1 (adding branch parameter to abstract Git::createFile) requires architectural decision - will address after maintainer feedback
1 parent 4b9c470 commit 724008f

File tree

2 files changed

+46
-38
lines changed

2 files changed

+46
-38
lines changed

src/VCS/Adapter/Git/Gitea.php

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -386,6 +386,12 @@ public function createPullRequest(string $owner, string $repositoryName, string
386386
$payload
387387
);
388388

389+
$responseHeaders = $response['headers'] ?? [];
390+
$responseHeadersStatusCode = $responseHeaders['status-code'] ?? 0;
391+
if ($responseHeadersStatusCode >= 400) {
392+
throw new Exception("Failed to create pull request: HTTP {$responseHeadersStatusCode}");
393+
}
394+
389395
$responseBody = $response['body'] ?? [];
390396

391397
return $responseBody;
@@ -448,6 +454,12 @@ public function getPullRequest(string $owner, string $repositoryName, int $pullR
448454

449455
$response = $this->call(self::METHOD_GET, $url, ['Authorization' => "token $this->accessToken"]);
450456

457+
$responseHeaders = $response['headers'] ?? [];
458+
$responseHeadersStatusCode = $responseHeaders['status-code'] ?? 0;
459+
if ($responseHeadersStatusCode >= 400) {
460+
throw new Exception("Failed to get pull request: HTTP {$responseHeadersStatusCode}");
461+
}
462+
451463
return $response['body'] ?? [];
452464
}
453465

tests/VCS/Adapter/GiteaTest.php

Lines changed: 34 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -103,45 +103,41 @@ public function testCommentWorkflow(): void
103103
$repositoryName = 'test-comment-workflow-' . \uniqid();
104104
$this->vcsAdapter->createRepository(self::$owner, $repositoryName, false);
105105

106-
$this->vcsAdapter->createFile(self::$owner, $repositoryName, 'README.md', '# Test');
107-
$this->vcsAdapter->createBranch(self::$owner, $repositoryName, 'comment-test', 'main');
108-
$this->vcsAdapter->createFile(self::$owner, $repositoryName, 'test.txt', 'test', 'Add test file', 'comment-test');
109-
110-
$pr = $this->vcsAdapter->createPullRequest(
111-
self::$owner,
112-
$repositoryName,
113-
'Comment Test PR',
114-
'comment-test',
115-
'main'
116-
);
117-
118-
$prNumber = $pr['number'] ?? 0;
119-
$this->assertGreaterThan(0, $prNumber);
106+
try {
107+
$this->vcsAdapter->createFile(self::$owner, $repositoryName, 'README.md', '# Test');
108+
$this->vcsAdapter->createBranch(self::$owner, $repositoryName, 'comment-test', 'main');
109+
$this->vcsAdapter->createFile(self::$owner, $repositoryName, 'test.txt', 'test', 'Add test file', 'comment-test');
120110

121-
$originalComment = 'This is a test comment';
122-
$commentId = $this->vcsAdapter->createComment(self::$owner, $repositoryName, $prNumber, $originalComment);
111+
$pr = $this->vcsAdapter->createPullRequest(
112+
self::$owner,
113+
$repositoryName,
114+
'Comment Test PR',
115+
'comment-test',
116+
'main'
117+
);
123118

124-
$this->assertNotEmpty($commentId);
125-
$this->assertIsString($commentId);
119+
$prNumber = $pr['number'] ?? 0;
120+
$this->assertGreaterThan(0, $prNumber);
126121

127-
// Test getComment
128-
$retrievedComment = $this->vcsAdapter->getComment(self::$owner, $repositoryName, $commentId);
122+
$originalComment = 'This is a test comment';
123+
$commentId = $this->vcsAdapter->createComment(self::$owner, $repositoryName, $prNumber, $originalComment);
129124

130-
$this->assertSame($originalComment, $retrievedComment);
131-
$this->assertIsString($commentId);
132-
$this->assertNotEmpty($commentId);
125+
$this->assertNotEmpty($commentId);
126+
$this->assertIsString($commentId);
133127

134-
// Test updateComment
135-
$updatedCommentText = 'This comment has been updated';
136-
$updatedCommentId = $this->vcsAdapter->updateComment(self::$owner, $repositoryName, (int)$commentId, $updatedCommentText);
128+
$retrievedComment = $this->vcsAdapter->getComment(self::$owner, $repositoryName, $commentId);
129+
$this->assertSame($originalComment, $retrievedComment);
137130

138-
$this->assertSame($commentId, $updatedCommentId);
131+
$updatedCommentText = 'This comment has been updated';
132+
$updatedCommentId = $this->vcsAdapter->updateComment(self::$owner, $repositoryName, (int)$commentId, $updatedCommentText);
139133

140-
// Verify the update
141-
$finalComment = $this->vcsAdapter->getComment(self::$owner, $repositoryName, $commentId);
142-
$this->assertSame($updatedCommentText, $finalComment);
134+
$this->assertSame($commentId, $updatedCommentId);
143135

144-
$this->vcsAdapter->deleteRepository(self::$owner, $repositoryName);
136+
$finalComment = $this->vcsAdapter->getComment(self::$owner, $repositoryName, $commentId);
137+
$this->assertSame($updatedCommentText, $finalComment);
138+
} finally {
139+
$this->vcsAdapter->deleteRepository(self::$owner, $repositoryName);
140+
}
145141
}
146142

147143
public function testGetComment(): void
@@ -196,11 +192,10 @@ public function testGetCommentInvalidId(): void
196192
$this->vcsAdapter->createRepository(self::$owner, $repositoryName, false);
197193
$this->vcsAdapter->createFile(self::$owner, $repositoryName, 'README.md', '# Test');
198194

199-
// Getting invalid comment should return empty string
200195
$result = $this->vcsAdapter->getComment(self::$owner, $repositoryName, '99999999');
201196

202197
$this->assertIsString($result);
203-
// May be empty or throw exception depending on API
198+
$this->assertSame('', $result);
204199

205200
$this->vcsAdapter->deleteRepository(self::$owner, $repositoryName);
206201
}
@@ -484,11 +479,12 @@ public function testGetPullRequestWithInvalidNumber(): void
484479
$this->vcsAdapter->createRepository(self::$owner, $repositoryName, false);
485480
$this->vcsAdapter->createFile(self::$owner, $repositoryName, 'README.md', '# Test');
486481

487-
// Try to get non-existent PR
488-
$result = $this->vcsAdapter->getPullRequest(self::$owner, $repositoryName, 99999);
489-
490-
// Should return empty or have error handling
491-
$this->assertIsArray($result);
482+
try {
483+
$this->expectException(\Exception::class);
484+
$this->vcsAdapter->getPullRequest(self::$owner, $repositoryName, 99999);
485+
} finally {
486+
$this->vcsAdapter->deleteRepository(self::$owner, $repositoryName);
487+
}
492488

493489
$this->vcsAdapter->deleteRepository(self::$owner, $repositoryName);
494490
}

0 commit comments

Comments
 (0)