From 6b80afe9341743bcca7b9b9eb903af55e5ca6be7 Mon Sep 17 00:00:00 2001 From: Matt Glaman Date: Tue, 14 Jul 2026 13:38:34 -0500 Subject: [PATCH 1/3] Verify all destination remotes before pushing artifact push:artifact only cloned and fetched from the first destination git URL, then pushed to the rest blind. Because every run generates a fresh commit SHA, any drift between destinations (branch left over on one remote, concurrent builds, a partial push failure) made later pushes fail with a non-fast-forward rejection that only a force push could clear. This hit PR/build branches constantly while main survived because merges serialize its pushes. Now the branch tip is fetched from every destination before the artifact is built. If tips differ but are ancestor-related, the build bases on the most advanced tip so every remote can fast-forward. If tips have truly diverged, the command aborts with an error naming each remote and its tip before anything is built. A push failure on one remote no longer skips the remaining remotes, which was how the drift started in the first place. Co-Authored-By: Claude Fable 5 --- src/Command/Push/PushArtifactCommand.php | 124 ++++++++-- .../Commands/Push/PushArtifactCommandTest.php | 227 ++++++++++++++++-- 2 files changed, 318 insertions(+), 33 deletions(-) diff --git a/src/Command/Push/PushArtifactCommand.php b/src/Command/Push/PushArtifactCommand.php index 858240fa..924ea56f 100644 --- a/src/Command/Push/PushArtifactCommand.php +++ b/src/Command/Push/PushArtifactCommand.php @@ -111,7 +111,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int ]); $this->checklist->addItem('Preparing artifact directory'); - $this->cloneSourceBranch($outputCallback, $artifactDir, $destinationGitUrls[0], $sourceGitBranch); + $this->cloneSourceBranch($outputCallback, $artifactDir, $destinationGitUrls, $sourceGitBranch); $this->checklist->completePreviousItem(); } @@ -175,8 +175,10 @@ private function determineDestinationGitUrls(): array /** * Prepare a directory to build the artifact. + * + * @param string[] $vcsUrls */ - private function cloneSourceBranch(Closure $outputCallback, string $artifactDir, string $vcsUrl, string $vcsPath): void + private function cloneSourceBranch(Closure $outputCallback, string $artifactDir, array $vcsUrls, string $vcsPath): void { $fs = $this->localMachineHelper->getFilesystem(); @@ -185,39 +187,63 @@ private function cloneSourceBranch(Closure $outputCallback, string $artifactDir, $outputCallback('out', "Initializing Git in $artifactDir"); $this->localMachineHelper->checkRequiredBinariesExist(['git']); + $printOutput = $this->output->getVerbosity() > OutputInterface::VERBOSITY_NORMAL; $process = $this->localMachineHelper->execute([ 'git', 'clone', '--depth=1', - $vcsUrl, + $vcsUrls[0], $artifactDir, - ], $outputCallback, null, ($this->output->getVerbosity() > OutputInterface::VERBOSITY_NORMAL)); + ], $outputCallback, null, $printOutput); if (!$process->isSuccessful()) { throw new AcquiaCliException('Failed to clone repository from the Cloud Platform: {message}', ['message' => $process->getErrorOutput()]); } - $process = $this->localMachineHelper->execute([ - 'git', - 'fetch', - '--depth=1', - '--update-head-ok', - $vcsUrl, - $vcsPath . ':' . $vcsPath, - ], $outputCallback, $artifactDir, ($this->output->getVerbosity() > OutputInterface::VERBOSITY_NORMAL)); - if (!$process->isSuccessful()) { - // Remote branch does not exist. Just create it locally. This will create - // the new branch off of the current commit. + + // Fetch the branch tip from every destination so out-of-sync remotes + // are detected before the artifact is built and pushed. + $tips = []; + foreach ($vcsUrls as $vcsUrl) { + $outputCallback('out', "Fetching $vcsPath from $vcsUrl"); + $process = $this->localMachineHelper->execute([ + 'git', + 'fetch', + '--depth=1', + $vcsUrl, + $vcsPath, + ], $outputCallback, $artifactDir, $printOutput); + if (!$process->isSuccessful()) { + // The branch does not exist on this remote yet. The push will + // create it. + continue; + } + $process = $this->localMachineHelper->execute([ + 'git', + 'rev-parse', + 'FETCH_HEAD', + ], null, $artifactDir, false); + if ($process->isSuccessful() && trim($process->getOutput()) !== '') { + $tips[$vcsUrl] = trim($process->getOutput()); + } + } + + if ($tips === []) { + // The branch does not exist on any remote. Just create it locally. + // This will create the new branch off of the current commit. $process = $this->localMachineHelper->execute([ 'git', 'checkout', '-b', $vcsPath, - ], $outputCallback, $artifactDir, ($this->output->getVerbosity() > OutputInterface::VERBOSITY_NORMAL)); + ], $outputCallback, $artifactDir, $printOutput); } else { + $baseTip = $this->determineBaseTip($tips, $vcsPath, $artifactDir); $process = $this->localMachineHelper->execute([ 'git', 'checkout', + '-B', $vcsPath, - ], $outputCallback, $artifactDir, ($this->output->getVerbosity() > OutputInterface::VERBOSITY_NORMAL)); + $baseTip, + ], $outputCallback, $artifactDir, $printOutput); } if (!$process->isSuccessful()) { throw new AcquiaCliException("Could not checkout $vcsPath branch locally: {message}", ['message' => $process->getErrorOutput() . $process->getOutput()]); @@ -392,12 +418,69 @@ private function generateCommitMessage(string $commitHash): array|string return "Automated commit by Acquia CLI (source commit: $commitHash)"; } + /** + * Pick the branch tip to base the artifact on. + * + * Ensures every destination can fast-forward to the artifact commit. If + * the tips differ but are ancestor-related, the most advanced tip wins. + * Truly diverged tips abort the push before anything is built. + * + * @param array $tips + */ + private function determineBaseTip(array $tips, string $vcsPath, string $artifactDir): string + { + $uniqueTips = array_values(array_unique($tips)); + if (count($uniqueTips) === 1) { + return $uniqueTips[0]; + } + + // The tips differ. Deepen the shallow history so ancestry between + // them can be established. + foreach (array_keys($tips) as $vcsUrl) { + $this->localMachineHelper->execute([ + 'git', + 'fetch', + '--deepen=50', + $vcsUrl, + $vcsPath, + ], null, $artifactDir, false); + } + foreach ($uniqueTips as $candidate) { + foreach ($uniqueTips as $other) { + if ($other === $candidate) { + continue; + } + $process = $this->localMachineHelper->execute([ + 'git', + 'merge-base', + '--is-ancestor', + $other, + $candidate, + ], null, $artifactDir, false); + if (!$process->isSuccessful()) { + continue 2; + } + } + return $candidate; + } + + $remoteTips = []; + foreach ($tips as $vcsUrl => $tip) { + $remoteTips[] = "$vcsUrl ($tip)"; + } + throw new AcquiaCliException('The destination git repositories are out of sync for the {branch} branch: {tips}. Reconcile them (e.g. delete the stale artifact branch from the out-of-date remote or push the desired tip to it) and try again.', [ + 'branch' => $vcsPath, + 'tips' => implode(', ', $remoteTips), + ]); + } + /** * Push the artifact. */ private function pushArtifact(Closure $outputCallback, string $artifactDir, array $vcsUrls, string $destGitBranch): void { $this->localMachineHelper->checkRequiredBinariesExist(['git']); + $failures = []; foreach ($vcsUrls as $vcsUrl) { $outputCallback('out', "Pushing changes to Acquia Git ($vcsUrl)"); $args = [ @@ -408,9 +491,14 @@ private function pushArtifact(Closure $outputCallback, string $artifactDir, arra ]; $process = $this->localMachineHelper->execute($args, $outputCallback, $artifactDir, ($this->output->getVerbosity() > OutputInterface::VERBOSITY_NORMAL)); if (!$process->isSuccessful()) { - throw new AcquiaCliException("Unable to push artifact: {message}", ['message' => $process->getOutput() . $process->getErrorOutput()]); + // Keep pushing to the remaining remotes so a single failure + // does not leave them further out of sync. + $failures[] = "$vcsUrl: " . $process->getOutput() . $process->getErrorOutput(); } } + if ($failures !== []) { + throw new AcquiaCliException("Unable to push artifact: {message}", ['message' => implode(PHP_EOL, $failures)]); + } } /** diff --git a/tests/phpunit/src/Commands/Push/PushArtifactCommandTest.php b/tests/phpunit/src/Commands/Push/PushArtifactCommandTest.php index d5b738c1..118e89a8 100644 --- a/tests/phpunit/src/Commands/Push/PushArtifactCommandTest.php +++ b/tests/phpunit/src/Commands/Push/PushArtifactCommandTest.php @@ -6,6 +6,7 @@ use Acquia\Cli\Command\CommandBase; use Acquia\Cli\Command\Push\PushArtifactCommand; +use Acquia\Cli\Exception\AcquiaCliException; use Acquia\Cli\Tests\Commands\Pull\PullCommandTestBase; use PHPUnit\Framework\Attributes\DataProvider; use Prophecy\Argument; @@ -169,6 +170,127 @@ public function testPushArtifactWithArgs(): void $this->assertStringContainsString('Pushing changes to Acquia Git (https://github.com/example2/cli.git)', $output); } + public function testPushArtifactToDivergedRemotes(): void + { + $destinationGitUrls = [ + 'https://github.com/example1/cli.git', + 'https://github.com/example2/cli.git', + ]; + $localMachineHelper = $this->mockLocalMachineHelper(); + touch(Path::join($this->projectDir, 'composer.json')); + mkdir(Path::join($this->projectDir, 'docroot')); + $this->createMockGitConfigFile(); + $fs = $this->prophet->prophesize(Filesystem::class); + $localMachineHelper->getFilesystem()->willReturn($fs); + $this->mockExecuteGitStatus(false, $localMachineHelper, $this->projectDir); + $this->mockGetLocalCommitHash($localMachineHelper, $this->projectDir, 'abc123'); + $localMachineHelper->checkRequiredBinariesExist(['git']) + ->shouldBeCalled(); + $artifactDir = Path::join(sys_get_temp_dir(), 'acli-push-artifact'); + $this->mockCloneShallow($localMachineHelper, 'master', $destinationGitUrls, $artifactDir, true, [ + $destinationGitUrls[0] => 'sha1', + $destinationGitUrls[1] => 'sha2', + ]); + $this->mockGitDeepen($localMachineHelper, 'master', $destinationGitUrls); + $this->mockGitMergeBase($localMachineHelper, 'sha2', 'sha1', false); + $this->mockGitMergeBase($localMachineHelper, 'sha1', 'sha2', false); + + $this->expectException(AcquiaCliException::class); + $this->expectExceptionMessageMatches('/out of sync/'); + $this->executeCommand([ + '--destination-git-branch' => 'master', + '--destination-git-urls' => $destinationGitUrls, + ]); + } + + public function testPushArtifactWithBranchMissingOnFirstRemote(): void + { + $destinationGitUrls = [ + 'https://github.com/example1/cli.git', + 'https://github.com/example2/cli.git', + ]; + $localMachineHelper = $this->mockLocalMachineHelper(); + $this->setUpPushArtifact($localMachineHelper, 'master', $destinationGitUrls, 'master:master', true, true, true, true, [ + $destinationGitUrls[0] => null, + $destinationGitUrls[1] => 'secondremotesha', + ]); + $this->executeCommand([ + '--destination-git-branch' => 'master', + '--destination-git-urls' => $destinationGitUrls, + ]); + + $output = $this->getDisplay(); + + $this->assertStringContainsString('Pushing changes to Acquia Git (https://github.com/example1/cli.git)', $output); + $this->assertStringContainsString('Pushing changes to Acquia Git (https://github.com/example2/cli.git)', $output); + } + + public function testPushArtifactWithRemoteBehind(): void + { + $destinationGitUrls = [ + 'https://github.com/example1/cli.git', + 'https://github.com/example2/cli.git', + ]; + $localMachineHelper = $this->mockLocalMachineHelper(); + $this->setUpPushArtifact($localMachineHelper, 'master', $destinationGitUrls, 'master:master', true, true, true, true, [ + $destinationGitUrls[0] => 'newsha', + $destinationGitUrls[1] => 'oldsha', + ]); + $this->mockGitDeepen($localMachineHelper, 'master', $destinationGitUrls); + $this->mockGitMergeBase($localMachineHelper, 'oldsha', 'newsha', true); + $this->mockGitCheckoutBase($localMachineHelper, 'master', 'newsha'); + $this->executeCommand([ + '--destination-git-branch' => 'master', + '--destination-git-urls' => $destinationGitUrls, + ]); + + $output = $this->getDisplay(); + + $this->assertStringContainsString('Pushing changes to Acquia Git (https://github.com/example1/cli.git)', $output); + $this->assertStringContainsString('Pushing changes to Acquia Git (https://github.com/example2/cli.git)', $output); + } + + public function testPushArtifactWithNewBranchOnAllRemotes(): void + { + $destinationGitUrls = [ + 'https://github.com/example1/cli.git', + 'https://github.com/example2/cli.git', + ]; + $localMachineHelper = $this->mockLocalMachineHelper(); + $this->setUpPushArtifact($localMachineHelper, 'feature-1-build', $destinationGitUrls, 'feature-1-build:feature-1-build', true, true, true, true, [ + $destinationGitUrls[0] => null, + $destinationGitUrls[1] => null, + ]); + $this->executeCommand([ + '--destination-git-branch' => 'feature-1-build', + '--destination-git-urls' => $destinationGitUrls, + ]); + + $output = $this->getDisplay(); + + $this->assertStringContainsString('Pushing changes to Acquia Git (https://github.com/example1/cli.git)', $output); + $this->assertStringContainsString('Pushing changes to Acquia Git (https://github.com/example2/cli.git)', $output); + } + + public function testPushArtifactPushFailureStillPushesRemainingRemotes(): void + { + $destinationGitUrls = [ + 'https://github.com/example1/cli.git', + 'https://github.com/example2/cli.git', + ]; + $localMachineHelper = $this->mockLocalMachineHelper(); + $this->setUpPushArtifact($localMachineHelper, 'master', $destinationGitUrls, 'master:master', true, true, false); + $artifactDir = Path::join(sys_get_temp_dir(), 'acli-push-artifact'); + $this->mockGitPush($destinationGitUrls, $localMachineHelper, $artifactDir, 'master:master', true, [$destinationGitUrls[0]]); + + $this->expectException(AcquiaCliException::class); + $this->expectExceptionMessageMatches('~https://github\.com/example1/cli\.git~'); + $this->executeCommand([ + '--destination-git-branch' => 'master', + '--destination-git-urls' => $destinationGitUrls, + ]); + } + public function testPushArtifactNoPush(): void { $applications = $this->mockRequest('getApplications'); @@ -243,7 +365,7 @@ public function testPushArtifactNoClone(): void $this->assertStringNotContainsString('Adding and committing changed files', $output); $this->assertStringNotContainsString('Pushing changes to Acquia Git (site@svn-3.hosted.acquia-sites.com:site.git)', $output); } - protected function setUpPushArtifact(ObjectProphecy $localMachineHelper, string $vcsPath, array $vcsUrls, string $destGitRef = 'master:master', bool $clone = true, bool $commit = true, bool $push = true, bool $printOutput = true): void + protected function setUpPushArtifact(ObjectProphecy $localMachineHelper, string $vcsPath, array $vcsUrls, string $destGitRef = 'master:master', bool $clone = true, bool $commit = true, bool $push = true, bool $printOutput = true, ?array $tips = null): void { touch(Path::join($this->projectDir, 'composer.json')); mkdir(Path::join($this->projectDir, 'docroot')); @@ -264,7 +386,7 @@ protected function setUpPushArtifact(ObjectProphecy $localMachineHelper, string if ($clone) { $this->mockLocalGitConfig($localMachineHelper, $artifactDir, $printOutput); - $this->mockCloneShallow($localMachineHelper, $vcsPath, $vcsUrls[0], $artifactDir, $printOutput); + $this->mockCloneShallow($localMachineHelper, $vcsPath, $vcsUrls, $artifactDir, $printOutput, $tips); } if ($commit) { $this->mockGitAddCommit($localMachineHelper, $artifactDir, $commitHash, $printOutput); @@ -274,35 +396,110 @@ protected function setUpPushArtifact(ObjectProphecy $localMachineHelper, string } } - protected function mockCloneShallow(ObjectProphecy $localMachineHelper, string $vcsPath, string $vcsUrl, string $artifactDir, bool $printOutput = true): void + /** + * @param array|null $tips + * Map of vcs url to the branch tip sha on that remote. A null value + * means the branch does not exist on that remote. Defaults to every + * url sharing the same tip. + */ + protected function mockCloneShallow(ObjectProphecy $localMachineHelper, string $vcsPath, array $vcsUrls, string $artifactDir, bool $printOutput = true, ?array $tips = null): void { + if ($tips === null) { + $tips = array_fill_keys($vcsUrls, 'mainbranchsha'); + } $process = $this->prophet->prophesize(Process::class); $process->isSuccessful()->willReturn(true)->shouldBeCalled(); $localMachineHelper->execute([ 'git', 'clone', '--depth=1', - $vcsUrl, + $vcsUrls[0], $artifactDir, ], Argument::type('callable'), null, $printOutput) ->willReturn($process->reveal())->shouldBeCalled(); - $localMachineHelper->execute([ - 'git', - 'fetch', - '--depth=1', - '--update-head-ok', - $vcsUrl, - $vcsPath . ':' . $vcsPath, - ], Argument::type('callable'), Argument::type('string'), $printOutput) - ->willReturn($process->reveal())->shouldBeCalled(); + + $revParseProcesses = []; + foreach ($vcsUrls as $vcsUrl) { + $tip = $tips[$vcsUrl]; + $fetchProcess = $this->mockProcess($tip !== null); + $localMachineHelper->execute([ + 'git', + 'fetch', + '--depth=1', + $vcsUrl, + $vcsPath, + ], Argument::type('callable'), Argument::type('string'), $printOutput) + ->willReturn($fetchProcess->reveal())->shouldBeCalled(); + if ($tip !== null) { + $revParseProcess = $this->mockProcess(); + $revParseProcess->getOutput()->willReturn($tip . PHP_EOL); + $revParseProcesses[] = $revParseProcess->reveal(); + } + } + if ($revParseProcesses !== []) { + $localMachineHelper->execute([ + 'git', + 'rev-parse', + 'FETCH_HEAD', + ], null, Argument::type('string'), false) + ->willReturn(...$revParseProcesses)->shouldBeCalled(); + } + + $uniqueTips = array_values(array_unique(array_filter($tips, static fn ($tip) => $tip !== null))); + if ($uniqueTips === []) { + $localMachineHelper->execute([ + 'git', + 'checkout', + '-b', + $vcsPath, + ], Argument::type('callable'), Argument::type('string'), $printOutput) + ->willReturn($process->reveal())->shouldBeCalled(); + } elseif (count($uniqueTips) === 1) { + $this->mockGitCheckoutBase($localMachineHelper, $vcsPath, $uniqueTips[0], $printOutput); + } + // Multiple distinct tips: the test mocks deepen, merge-base, and + // checkout calls itself. + } + + protected function mockGitCheckoutBase(ObjectProphecy $localMachineHelper, string $vcsPath, string $baseTip, bool $printOutput = true): void + { + $process = $this->mockProcess(); $localMachineHelper->execute([ 'git', 'checkout', + '-B', $vcsPath, + $baseTip, ], Argument::type('callable'), Argument::type('string'), $printOutput) ->willReturn($process->reveal())->shouldBeCalled(); } + protected function mockGitDeepen(ObjectProphecy $localMachineHelper, string $vcsPath, array $vcsUrls): void + { + foreach ($vcsUrls as $vcsUrl) { + $localMachineHelper->execute([ + 'git', + 'fetch', + '--deepen=50', + $vcsUrl, + $vcsPath, + ], null, Argument::type('string'), false) + ->willReturn($this->mockProcess()->reveal())->shouldBeCalled(); + } + } + + protected function mockGitMergeBase(ObjectProphecy $localMachineHelper, string $ancestor, string $descendant, bool $isAncestor): void + { + $localMachineHelper->execute([ + 'git', + 'merge-base', + '--is-ancestor', + $ancestor, + $descendant, + ], null, Argument::type('string'), false) + ->willReturn($this->mockProcess($isAncestor)->reveal())->shouldBeCalled(); + } + protected function mockLocalGitConfig(ObjectProphecy $localMachineHelper, string $artifactDir, bool $printOutput = true): void { $process = $this->prophet->prophesize(Process::class); @@ -406,10 +603,10 @@ protected function mockReadComposerJson(ObjectProphecy $localMachineHelper, stri ->willReturn($composerJson); } - protected function mockGitPush(array $gitUrls, ObjectProphecy $localMachineHelper, string $artifactDir, string $destGitRef, bool $printOutput): void + protected function mockGitPush(array $gitUrls, ObjectProphecy $localMachineHelper, string $artifactDir, string $destGitRef, bool $printOutput, array $failingUrls = []): void { - $process = $this->mockProcess(); foreach ($gitUrls as $gitUrl) { + $process = $this->mockProcess(!in_array($gitUrl, $failingUrls, true)); $localMachineHelper->execute([ 'git', 'push', From a23ed33a7c26b690763e8bebc8aa2d40c49cb221 Mon Sep 17 00:00:00 2001 From: Matt Glaman Date: Fri, 31 Jul 2026 09:55:47 -0500 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/Command/Push/PushArtifactCommand.php | 36 ++++++++++++++++--- .../Commands/Push/PushArtifactCommandTest.php | 2 +- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/src/Command/Push/PushArtifactCommand.php b/src/Command/Push/PushArtifactCommand.php index 924ea56f..eea752c0 100644 --- a/src/Command/Push/PushArtifactCommand.php +++ b/src/Command/Push/PushArtifactCommand.php @@ -221,9 +221,21 @@ private function cloneSourceBranch(Closure $outputCallback, string $artifactDir, 'rev-parse', 'FETCH_HEAD', ], null, $artifactDir, false); - if ($process->isSuccessful() && trim($process->getOutput()) !== '') { - $tips[$vcsUrl] = trim($process->getOutput()); + if (!$process->isSuccessful()) { + throw new AcquiaCliException('Failed to resolve fetched tip for the {branch} branch from {url}: {message}', [ + 'branch' => $vcsPath, + 'url' => $vcsUrl, + 'message' => $process->getErrorOutput() . $process->getOutput(), + ]); + } + $tip = trim($process->getOutput()); + if ($tip === '') { + throw new AcquiaCliException('Failed to resolve fetched tip for the {branch} branch from {url}: empty output', [ + 'branch' => $vcsPath, + 'url' => $vcsUrl, + ]); } + $tips[$vcsUrl] = $tip; } if ($tips === []) { @@ -437,13 +449,20 @@ private function determineBaseTip(array $tips, string $vcsPath, string $artifact // The tips differ. Deepen the shallow history so ancestry between // them can be established. foreach (array_keys($tips) as $vcsUrl) { - $this->localMachineHelper->execute([ + $process = $this->localMachineHelper->execute([ 'git', 'fetch', '--deepen=50', $vcsUrl, $vcsPath, ], null, $artifactDir, false); + if (!$process->isSuccessful()) { + throw new AcquiaCliException('Failed to deepen history for the {branch} branch from {url}: {message}', [ + 'branch' => $vcsPath, + 'url' => $vcsUrl, + 'message' => $process->getErrorOutput() . $process->getOutput(), + ]); + } } foreach ($uniqueTips as $candidate) { foreach ($uniqueTips as $other) { @@ -457,9 +476,18 @@ private function determineBaseTip(array $tips, string $vcsPath, string $artifact $other, $candidate, ], null, $artifactDir, false); - if (!$process->isSuccessful()) { + $exitCode = $process->getExitCode(); + if ($exitCode === 0) { + continue; + } + if ($exitCode === 1) { continue 2; } + throw new AcquiaCliException('Failed to compare ancestry between {other} and {candidate}: {message}', [ + 'other' => $other, + 'candidate' => $candidate, + 'message' => $process->getErrorOutput() . $process->getOutput(), + ]); } return $candidate; } diff --git a/tests/phpunit/src/Commands/Push/PushArtifactCommandTest.php b/tests/phpunit/src/Commands/Push/PushArtifactCommandTest.php index 118e89a8..8d14ac91 100644 --- a/tests/phpunit/src/Commands/Push/PushArtifactCommandTest.php +++ b/tests/phpunit/src/Commands/Push/PushArtifactCommandTest.php @@ -420,7 +420,7 @@ protected function mockCloneShallow(ObjectProphecy $localMachineHelper, string $ $revParseProcesses = []; foreach ($vcsUrls as $vcsUrl) { - $tip = $tips[$vcsUrl]; + $tip = $tips[$vcsUrl] ?? null; $fetchProcess = $this->mockProcess($tip !== null); $localMachineHelper->execute([ 'git', From ab67f3155687c2963b0e5330a31dd5f9fb8fc0d5 Mon Sep 17 00:00:00 2001 From: Matt Glaman Date: Fri, 31 Jul 2026 10:10:08 -0500 Subject: [PATCH 3/3] Cover artifact push error paths and remote sync failures Add tests for unresolvable and empty fetched tips, failed history deepening, and unexpected merge-base exit codes. Assert the full out-of-sync message and the fetch progress output so the covered mutation score stays at 100%. Co-Authored-By: Claude Opus 5 (1M context) --- .../Commands/Push/PushArtifactCommandTest.php | 259 ++++++++++++++---- 1 file changed, 212 insertions(+), 47 deletions(-) diff --git a/tests/phpunit/src/Commands/Push/PushArtifactCommandTest.php b/tests/phpunit/src/Commands/Push/PushArtifactCommandTest.php index a40f805d..63da2414 100644 --- a/tests/phpunit/src/Commands/Push/PushArtifactCommandTest.php +++ b/tests/phpunit/src/Commands/Push/PushArtifactCommandTest.php @@ -21,6 +21,10 @@ */ class PushArtifactCommandTest extends PullCommandTestBase { + private const PROCESS_OUTPUT = 'process stdout.'; + + private const PROCESS_ERROR_OUTPUT = 'process stderr.'; + protected function createCommand(): CommandBase { return $this->injectCommand(PushArtifactCommand::class); @@ -166,6 +170,8 @@ public function testPushArtifactWithArgs(): void $output = $this->getDisplay(); + $this->assertStringContainsString('Fetching master from https://github.com/example1/cli.git', $output); + $this->assertStringContainsString('Fetching master from https://github.com/example2/cli.git', $output); $this->assertStringContainsString('Pushing changes to Acquia Git (https://github.com/example1/cli.git)', $output); $this->assertStringContainsString('Pushing changes to Acquia Git (https://github.com/example2/cli.git)', $output); } @@ -177,16 +183,7 @@ public function testPushArtifactToDivergedRemotes(): void 'https://github.com/example2/cli.git', ]; $localMachineHelper = $this->mockLocalMachineHelper(); - touch(Path::join($this->projectDir, 'composer.json')); - mkdir(Path::join($this->projectDir, 'docroot')); - $this->createMockGitConfigFile(); - $fs = $this->prophet->prophesize(Filesystem::class); - $localMachineHelper->getFilesystem()->willReturn($fs); - $this->mockExecuteGitStatus(false, $localMachineHelper, $this->projectDir); - $this->mockGetLocalCommitHash($localMachineHelper, $this->projectDir, 'abc123'); - $localMachineHelper->checkRequiredBinariesExist(['git']) - ->shouldBeCalled(); - $artifactDir = Path::join(sys_get_temp_dir(), 'acli-push-artifact'); + $artifactDir = $this->mockArtifactSourceCheckout($localMachineHelper); $this->mockCloneShallow($localMachineHelper, 'master', $destinationGitUrls, $artifactDir, true, [ $destinationGitUrls[0] => 'sha1', $destinationGitUrls[1] => 'sha2', @@ -196,7 +193,116 @@ public function testPushArtifactToDivergedRemotes(): void $this->mockGitMergeBase($localMachineHelper, 'sha1', 'sha2', false); $this->expectException(AcquiaCliException::class); - $this->expectExceptionMessageMatches('/out of sync/'); + $this->expectExceptionMessage('The destination git repositories are out of sync for the master branch: https://github.com/example1/cli.git (sha1), https://github.com/example2/cli.git (sha2).'); + $this->executeCommand([ + '--destination-git-branch' => 'master', + '--destination-git-urls' => $destinationGitUrls, + ]); + } + + public function testPushArtifactToRemoteDivergedFromOneOfThreeRemotes(): void + { + $destinationGitUrls = [ + 'https://github.com/example1/cli.git', + 'https://github.com/example2/cli.git', + 'https://github.com/example3/cli.git', + ]; + $localMachineHelper = $this->mockLocalMachineHelper(); + $artifactDir = $this->mockArtifactSourceCheckout($localMachineHelper); + $this->mockCloneShallow($localMachineHelper, 'master', $destinationGitUrls, $artifactDir, true, [ + $destinationGitUrls[0] => 'newsha', + $destinationGitUrls[1] => 'oldsha', + $destinationGitUrls[2] => 'divergedsha', + ]); + $this->mockGitDeepen($localMachineHelper, 'master', $destinationGitUrls); + // The second remote is behind the first, so the first remains a + // candidate until the diverged third remote rules it out. + $this->mockGitMergeBase($localMachineHelper, 'oldsha', 'newsha', true); + $this->mockGitMergeBase($localMachineHelper, 'divergedsha', 'newsha', false); + $this->mockGitMergeBase($localMachineHelper, 'newsha', 'oldsha', false); + $this->mockGitMergeBase($localMachineHelper, 'newsha', 'divergedsha', false); + + $this->expectException(AcquiaCliException::class); + $this->expectExceptionMessage('The destination git repositories are out of sync for the master branch'); + $this->executeCommand([ + '--destination-git-branch' => 'master', + '--destination-git-urls' => $destinationGitUrls, + ]); + } + + public function testPushArtifactFailsWhenFetchedTipCannotBeResolved(): void + { + $destinationGitUrls = ['https://github.com/example1/cli.git']; + $localMachineHelper = $this->mockLocalMachineHelper(); + $artifactDir = $this->mockArtifactSourceCheckout($localMachineHelper); + $this->mockGitCloneShallow($localMachineHelper, $destinationGitUrls[0], $artifactDir); + $this->mockGitFetchBranch($localMachineHelper, 'master', $destinationGitUrls[0]); + $this->mockGitRevParse($localMachineHelper, ['sha1'], false); + + $this->expectException(AcquiaCliException::class); + $this->expectExceptionMessage('Failed to resolve fetched tip for the master branch from https://github.com/example1/cli.git: ' . self::PROCESS_ERROR_OUTPUT . self::PROCESS_OUTPUT); + $this->executeCommand([ + '--destination-git-branch' => 'master', + '--destination-git-urls' => $destinationGitUrls, + ]); + } + + public function testPushArtifactFailsWhenFetchedTipIsEmpty(): void + { + $destinationGitUrls = ['https://github.com/example1/cli.git']; + $localMachineHelper = $this->mockLocalMachineHelper(); + $artifactDir = $this->mockArtifactSourceCheckout($localMachineHelper); + $this->mockGitCloneShallow($localMachineHelper, $destinationGitUrls[0], $artifactDir); + $this->mockGitFetchBranch($localMachineHelper, 'master', $destinationGitUrls[0]); + $this->mockGitRevParse($localMachineHelper, ['']); + + $this->expectException(AcquiaCliException::class); + $this->expectExceptionMessage('Failed to resolve fetched tip for the master branch from https://github.com/example1/cli.git: empty output'); + $this->executeCommand([ + '--destination-git-branch' => 'master', + '--destination-git-urls' => $destinationGitUrls, + ]); + } + + public function testPushArtifactFailsWhenHistoryCannotBeDeepened(): void + { + $destinationGitUrls = [ + 'https://github.com/example1/cli.git', + 'https://github.com/example2/cli.git', + ]; + $localMachineHelper = $this->mockLocalMachineHelper(); + $artifactDir = $this->mockArtifactSourceCheckout($localMachineHelper); + $this->mockCloneShallow($localMachineHelper, 'master', $destinationGitUrls, $artifactDir, true, [ + $destinationGitUrls[0] => 'sha1', + $destinationGitUrls[1] => 'sha2', + ]); + $this->mockGitDeepen($localMachineHelper, 'master', [$destinationGitUrls[0]], false); + + $this->expectException(AcquiaCliException::class); + $this->expectExceptionMessage('Failed to deepen history for the master branch from https://github.com/example1/cli.git: ' . self::PROCESS_ERROR_OUTPUT . self::PROCESS_OUTPUT); + $this->executeCommand([ + '--destination-git-branch' => 'master', + '--destination-git-urls' => $destinationGitUrls, + ]); + } + + public function testPushArtifactFailsWhenAncestryCannotBeCompared(): void + { + $destinationGitUrls = [ + 'https://github.com/example1/cli.git', + 'https://github.com/example2/cli.git', + ]; + $localMachineHelper = $this->mockLocalMachineHelper(); + $artifactDir = $this->mockArtifactSourceCheckout($localMachineHelper); + $this->mockCloneShallow($localMachineHelper, 'master', $destinationGitUrls, $artifactDir, true, [ + $destinationGitUrls[0] => 'sha1', + $destinationGitUrls[1] => 'sha2', + ]); + $this->mockGitDeepen($localMachineHelper, 'master', $destinationGitUrls); + $this->mockGitMergeBaseExitCode($localMachineHelper, 'sha2', 'sha1', 128); + + $this->expectException(AcquiaCliException::class); + $this->expectExceptionMessage('Failed to compare ancestry between sha2 and sha1: ' . self::PROCESS_ERROR_OUTPUT . self::PROCESS_OUTPUT); $this->executeCommand([ '--destination-git-branch' => 'master', '--destination-git-urls' => $destinationGitUrls, @@ -284,7 +390,7 @@ public function testPushArtifactPushFailureStillPushesRemainingRemotes(): void $this->mockGitPush($destinationGitUrls, $localMachineHelper, $artifactDir, 'master:master', true, [$destinationGitUrls[0]]); $this->expectException(AcquiaCliException::class); - $this->expectExceptionMessageMatches('~https://github\.com/example1/cli\.git~'); + $this->expectExceptionMessage('Unable to push artifact: https://github.com/example1/cli.git: ' . self::PROCESS_OUTPUT . self::PROCESS_ERROR_OUTPUT); $this->executeCommand([ '--destination-git-branch' => 'master', '--destination-git-urls' => $destinationGitUrls, @@ -431,42 +537,18 @@ protected function mockCloneShallow(ObjectProphecy $localMachineHelper, string $ if ($tips === null) { $tips = array_fill_keys($vcsUrls, 'mainbranchsha'); } - $process = $this->prophet->prophesize(Process::class); - $process->isSuccessful()->willReturn(true)->shouldBeCalled(); - $localMachineHelper->execute([ - 'git', - 'clone', - '--depth=1', - $vcsUrls[0], - $artifactDir, - ], Argument::type('callable'), null, $printOutput) - ->willReturn($process->reveal())->shouldBeCalled(); + $this->mockGitCloneShallow($localMachineHelper, $vcsUrls[0], $artifactDir, $printOutput); - $revParseProcesses = []; + $revParseTips = []; foreach ($vcsUrls as $vcsUrl) { $tip = $tips[$vcsUrl] ?? null; - $fetchProcess = $this->mockProcess($tip !== null); - $localMachineHelper->execute([ - 'git', - 'fetch', - '--depth=1', - $vcsUrl, - $vcsPath, - ], Argument::type('callable'), Argument::type('string'), $printOutput) - ->willReturn($fetchProcess->reveal())->shouldBeCalled(); + $this->mockGitFetchBranch($localMachineHelper, $vcsPath, $vcsUrl, $printOutput, $tip !== null); if ($tip !== null) { - $revParseProcess = $this->mockProcess(); - $revParseProcess->getOutput()->willReturn($tip . PHP_EOL); - $revParseProcesses[] = $revParseProcess->reveal(); + $revParseTips[] = $tip; } } - if ($revParseProcesses !== []) { - $localMachineHelper->execute([ - 'git', - 'rev-parse', - 'FETCH_HEAD', - ], null, Argument::type('string'), false) - ->willReturn(...$revParseProcesses)->shouldBeCalled(); + if ($revParseTips !== []) { + $this->mockGitRevParse($localMachineHelper, $revParseTips); } $uniqueTips = array_values(array_unique(array_filter($tips, static fn ($tip) => $tip !== null))); @@ -477,7 +559,7 @@ protected function mockCloneShallow(ObjectProphecy $localMachineHelper, string $ '-b', $vcsPath, ], Argument::type('callable'), Argument::type('string'), $printOutput) - ->willReturn($process->reveal())->shouldBeCalled(); + ->willReturn($this->mockProcess()->reveal())->shouldBeCalled(); } elseif (count($uniqueTips) === 1) { $this->mockGitCheckoutBase($localMachineHelper, $vcsPath, $uniqueTips[0], $printOutput); } @@ -485,6 +567,72 @@ protected function mockCloneShallow(ObjectProphecy $localMachineHelper, string $ // checkout calls itself. } + /** + * Mock everything the command does before it clones the destination. + * + * @return string + * The artifact directory. + */ + protected function mockArtifactSourceCheckout(ObjectProphecy $localMachineHelper): string + { + touch(Path::join($this->projectDir, 'composer.json')); + mkdir(Path::join($this->projectDir, 'docroot')); + $this->createMockGitConfigFile(); + $fs = $this->prophet->prophesize(Filesystem::class); + $localMachineHelper->getFilesystem()->willReturn($fs); + $this->mockExecuteGitStatus(false, $localMachineHelper, $this->projectDir); + $this->mockGetLocalCommitHash($localMachineHelper, $this->projectDir, 'abc123'); + $localMachineHelper->checkRequiredBinariesExist(['git']) + ->shouldBeCalled(); + return Path::join(sys_get_temp_dir(), 'acli-push-artifact'); + } + + protected function mockGitCloneShallow(ObjectProphecy $localMachineHelper, string $vcsUrl, string $artifactDir, bool $printOutput = true): void + { + $localMachineHelper->execute([ + 'git', + 'clone', + '--depth=1', + $vcsUrl, + $artifactDir, + ], Argument::type('callable'), null, $printOutput) + ->willReturn($this->mockProcess()->reveal())->shouldBeCalled(); + } + + protected function mockGitFetchBranch(ObjectProphecy $localMachineHelper, string $vcsPath, string $vcsUrl, bool $printOutput = true, bool $success = true): void + { + $localMachineHelper->execute([ + 'git', + 'fetch', + '--depth=1', + $vcsUrl, + $vcsPath, + ], Argument::type('callable'), Argument::type('string'), $printOutput) + ->willReturn($this->mockProcess($success)->reveal())->shouldBeCalled(); + } + + /** + * @param string[] $tips + * The sha each consecutive rev-parse call resolves to. + */ + protected function mockGitRevParse(ObjectProphecy $localMachineHelper, array $tips, bool $success = true): void + { + $processes = []; + foreach ($tips as $tip) { + $process = $success ? $this->mockProcess() : $this->mockFailedProcess(); + if ($success) { + $process->getOutput()->willReturn($tip === '' ? '' : $tip . PHP_EOL); + } + $processes[] = $process->reveal(); + } + $localMachineHelper->execute([ + 'git', + 'rev-parse', + 'FETCH_HEAD', + ], null, Argument::type('string'), false) + ->willReturn(...$processes)->shouldBeCalled(); + } + protected function mockGitCheckoutBase(ObjectProphecy $localMachineHelper, string $vcsPath, string $baseTip, bool $printOutput = true): void { $process = $this->mockProcess(); @@ -498,9 +646,10 @@ protected function mockGitCheckoutBase(ObjectProphecy $localMachineHelper, strin ->willReturn($process->reveal())->shouldBeCalled(); } - protected function mockGitDeepen(ObjectProphecy $localMachineHelper, string $vcsPath, array $vcsUrls): void + protected function mockGitDeepen(ObjectProphecy $localMachineHelper, string $vcsPath, array $vcsUrls, bool $success = true): void { foreach ($vcsUrls as $vcsUrl) { + $process = $success ? $this->mockProcess() : $this->mockFailedProcess(); $localMachineHelper->execute([ 'git', 'fetch', @@ -508,12 +657,27 @@ protected function mockGitDeepen(ObjectProphecy $localMachineHelper, string $vcs $vcsUrl, $vcsPath, ], null, Argument::type('string'), false) - ->willReturn($this->mockProcess()->reveal())->shouldBeCalled(); + ->willReturn($process->reveal())->shouldBeCalled(); } } + protected function mockFailedProcess(): ObjectProphecy + { + $process = $this->mockProcess(false); + $process->getOutput()->willReturn(self::PROCESS_OUTPUT); + $process->getErrorOutput()->willReturn(self::PROCESS_ERROR_OUTPUT); + return $process; + } + protected function mockGitMergeBase(ObjectProphecy $localMachineHelper, string $ancestor, string $descendant, bool $isAncestor): void { + $this->mockGitMergeBaseExitCode($localMachineHelper, $ancestor, $descendant, $isAncestor ? 0 : 1); + } + + protected function mockGitMergeBaseExitCode(ObjectProphecy $localMachineHelper, string $ancestor, string $descendant, int $exitCode): void + { + $process = $exitCode === 0 ? $this->mockProcess() : $this->mockFailedProcess(); + $process->getExitCode()->willReturn($exitCode); $localMachineHelper->execute([ 'git', 'merge-base', @@ -521,7 +685,7 @@ protected function mockGitMergeBase(ObjectProphecy $localMachineHelper, string $ $ancestor, $descendant, ], null, Argument::type('string'), false) - ->willReturn($this->mockProcess($isAncestor)->reveal())->shouldBeCalled(); + ->willReturn($process->reveal())->shouldBeCalled(); } protected function mockLocalGitConfig(ObjectProphecy $localMachineHelper, string $artifactDir, bool $printOutput = true): void @@ -646,7 +810,8 @@ protected function mockReadComposerJson(ObjectProphecy $localMachineHelper, stri protected function mockGitPush(array $gitUrls, ObjectProphecy $localMachineHelper, string $artifactDir, string $destGitRef, bool $printOutput, array $failingUrls = []): void { foreach ($gitUrls as $gitUrl) { - $process = $this->mockProcess(!in_array($gitUrl, $failingUrls, true)); + $succeeds = !in_array($gitUrl, $failingUrls, true); + $process = $succeeds ? $this->mockProcess() : $this->mockFailedProcess(); $localMachineHelper->execute([ 'git', 'push',