Skip to content

Commit 446942a

Browse files
frankdekkerfrankdekker
authored
Add recoverable review diff service (#988)
* Add RecoverableReviewDiffService.php * Add RecoverableReviewDiffService.php * Add RecoverableReviewDiffService.php * Add coverage
1 parent 265a426 commit 446942a

File tree

3 files changed

+143
-1
lines changed

3 files changed

+143
-1
lines changed

‎config/services.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
use DR\Review\Service\Git\GitRepositoryService;
4242
use DR\Review\Service\Git\Review\ReviewDiffService\CacheableReviewDiffService;
4343
use DR\Review\Service\Git\Review\ReviewDiffService\LockableReviewDiffService;
44+
use DR\Review\Service\Git\Review\ReviewDiffService\RecoverableReviewDiffService;
4445
use DR\Review\Service\Git\Review\ReviewDiffService\ReviewDiffService;
4546
use DR\Review\Service\Git\Review\ReviewDiffService\ReviewDiffServiceInterface;
4647
use DR\Review\Service\Git\Review\Strategy\BasicCherryPickStrategy;
@@ -191,7 +192,8 @@
191192
$services->set(HesitantCherryPickStrategy::class)->tag('review_diff_strategy', ['priority' => 10]);
192193
$services->set('review.diff.service', ReviewDiffService::class)->arg('$reviewDiffStrategies', tagged_iterator('review_diff_strategy'));
193194

194-
$services->set('lock.review.diff.service', LockableReviewDiffService::class)->arg('$diffService', service('review.diff.service'));
195+
$services->set('recoverable.review.diff.service', RecoverableReviewDiffService::class)->arg('$diffService', service('review.diff.service'));
196+
$services->set('lock.review.diff.service', LockableReviewDiffService::class)->arg('$diffService', service('recoverable.review.diff.service'));
195197
$services->set(ReviewDiffServiceInterface::class, CacheableReviewDiffService::class)->arg('$diffService', service('lock.review.diff.service'));
196198
$services->set(ReviewRouter::class)->decorate('router')->args([service('.inner')]);
197199
$services->set(CodeReviewFileService::class)->arg('$revisionCache', service(CacheInterface::class . ' $revisionCache'));

‎src/Service/Git/Review/ReviewDiffService/RecoverableReviewDiffService.php‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
<?php
2+
declare(strict_types=1);
3+
4+
namespace DR\Review\Service\Git\Review\ReviewDiffService;
5+
6+
use DR\Review\Entity\Repository\Repository;
7+
use DR\Review\Entity\Review\CodeReview;
8+
use DR\Review\Service\Git\Review\FileDiffOptions;
9+
use Psr\Log\LoggerAwareInterface;
10+
use Psr\Log\LoggerAwareTrait;
11+
use Symfony\Component\Process\Exception\ProcessFailedException;
12+
13+
class RecoverableReviewDiffService implements ReviewDiffServiceInterface, LoggerAwareInterface
14+
{
15+
use LoggerAwareTrait;
16+
17+
public function __construct(private readonly ReviewDiffServiceInterface $diffService)
18+
{
19+
}
20+
21+
/**
22+
* @inheritDoc
23+
*/
24+
public function getDiffForRevisions(Repository $repository, array $revisions, ?FileDiffOptions $options = null): array
25+
{
26+
return $this->diffService->getDiffForRevisions($repository, $revisions, $options);
27+
}
28+
29+
/**
30+
* The target branch of the review might be stale, try to reset it to the main branch if the diff fails.
31+
* @inheritDoc
32+
*/
33+
public function getDiffForBranch(CodeReview $review, array $revisions, string $branchName, ?FileDiffOptions $options = null): array
34+
{
35+
try {
36+
return $this->diffService->getDiffForBranch($review, $revisions, $branchName, $options);
37+
} catch (ProcessFailedException $exception) {
38+
if ($review->getTargetBranch() === $review->getRepository()->getMainBranchName()) {
39+
throw $exception;
40+
}
41+
42+
$this->logger?->notice('Failed to get diff for branch, trying to reset target branch', ['exception' => $exception]);
43+
$review->setTargetBranch($review->getRepository()->getMainBranchName());
44+
}
45+
46+
return $this->diffService->getDiffForBranch($review, $revisions, $branchName, $options);
47+
}
48+
}

‎tests/Unit/Service/Git/Review/ReviewDiffService/RecoverableReviewDiffServiceTest.php‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
<?php
2+
declare(strict_types=1);
3+
4+
namespace DR\Review\Tests\Unit\Service\Git\Review\ReviewDiffService;
5+
6+
use DR\Review\Entity\Git\Diff\DiffComparePolicy;
7+
use DR\Review\Entity\Repository\Repository;
8+
use DR\Review\Entity\Review\CodeReview;
9+
use DR\Review\Entity\Revision\Revision;
10+
use DR\Review\Service\Git\Review\FileDiffOptions;
11+
use DR\Review\Service\Git\Review\ReviewDiffService\RecoverableReviewDiffService;
12+
use DR\Review\Service\Git\Review\ReviewDiffService\ReviewDiffServiceInterface;
13+
use DR\Review\Tests\AbstractTestCase;
14+
use PHPUnit\Framework\Attributes\CoversClass;
15+
use PHPUnit\Framework\MockObject\MockObject;
16+
use stdClass;
17+
use Symfony\Component\Process\Exception\ProcessFailedException;
18+
use Throwable;
19+
20+
#[CoversClass(RecoverableReviewDiffService::class)]
21+
class RecoverableReviewDiffServiceTest extends AbstractTestCase
22+
{
23+
private ReviewDiffServiceInterface&MockObject $diffService;
24+
private RecoverableReviewDiffService $service;
25+
26+
protected function setUp(): void
27+
{
28+
parent::setUp();
29+
$this->diffService = $this->createMock(ReviewDiffServiceInterface::class);
30+
$this->service = new RecoverableReviewDiffService($this->diffService);
31+
}
32+
33+
/**
34+
* @throws Throwable
35+
*/
36+
public function testGetDiffForRevisions(): void
37+
{
38+
$repository = new Repository();
39+
$revisions = [new Revision()];
40+
$options = new FileDiffOptions(5, DiffComparePolicy::ALL);
41+
42+
$this->diffService->expects(self::once())->method('getDiffForRevisions')->with($repository, $revisions, $options);
43+
44+
$this->service->getDiffForRevisions($repository, $revisions, $options);
45+
}
46+
47+
/**
48+
* @throws Throwable
49+
*/
50+
public function testGetDiffForBranchExpectTwoAttempts(): void
51+
{
52+
$callCount = new stdClass();
53+
$callCount->count = 0;
54+
$repository = (new Repository())->setMainBranchName('master');
55+
$review = (new CodeReview())->setTargetBranch('foobar')->setRepository($repository);
56+
$revisions = [new Revision()];
57+
$options = new FileDiffOptions(5, DiffComparePolicy::ALL);
58+
$branchName = 'branch';
59+
60+
$exception = $this->createMock(ProcessFailedException::class);
61+
62+
$this->diffService->expects(self::exactly(2))->method('getDiffForBranch')
63+
->with($review, $revisions, $branchName, $options)
64+
->willThrowException($exception);
65+
66+
$this->expectException(ProcessFailedException::class);
67+
$this->service->getDiffForBranch($review, $revisions, $branchName, $options);
68+
}
69+
70+
/**
71+
* @throws Throwable
72+
*/
73+
public function testGetDiffForBranchExpectOneAttemptOnMasterBranch(): void
74+
{
75+
$callCount = new stdClass();
76+
$callCount->count = 0;
77+
$repository = (new Repository())->setMainBranchName('master');
78+
$review = (new CodeReview())->setTargetBranch('master')->setRepository($repository);
79+
$revisions = [new Revision()];
80+
$options = new FileDiffOptions(5, DiffComparePolicy::ALL);
81+
$branchName = 'branch';
82+
83+
$exception = $this->createMock(ProcessFailedException::class);
84+
85+
$this->diffService->expects(self::once())->method('getDiffForBranch')
86+
->with($review, $revisions, $branchName, $options)
87+
->willThrowException($exception);
88+
89+
$this->expectException(ProcessFailedException::class);
90+
$this->service->getDiffForBranch($review, $revisions, $branchName, $options);
91+
}
92+
}

0 commit comments

Comments
 (0)