From 51c82edbb8d33b9d39f8e4ee9af8f4abf9f18c47 Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Mon, 7 Sep 2026 13:19:00 +0530 Subject: [PATCH] test(vcs): assert what a provider refuses instead of skipping A declared capability dropped the test that needed it. Where the adapter documents a refusal, the shared test now asserts it, and where the provider answers differently the test asserts that answer. Four behaviours the sharing surfaced: - Gitea says 'synchronized' for a pushed head. Consumers only act on 'synchronize', so a pull request update was read as an unknown action. - GitLab raised a bare Exception for a repository that does not exist, and answered the old path of a deleted one through a redirect. - GitHub asked for a user with no credential, the only call in the adapter that did. - getInstallationRepository() was stubbed three times over; the refusal belongs on Git, next to the other five. The pull request action mapping and the head commit test move into the unit tier, leaving no test of its own in GitLab's class. Co-Authored-By: Claude Opus 5 --- packages/vcs/docs/add-new-vcs-adapter.md | 17 +- packages/vcs/src/VCS/Adapter/Git.php | 13 + .../vcs/src/VCS/Adapter/Git/Bitbucket.php | 5 - packages/vcs/src/VCS/Adapter/Git/GitHub.php | 16 +- packages/vcs/src/VCS/Adapter/Git/GitLab.php | 19 +- packages/vcs/src/VCS/Adapter/Git/Gitea.php | 22 +- packages/vcs/src/VCS/Adapter/Git/Gogs.php | 14 +- packages/vcs/tests/E2E/Base.php | 306 +++++++++++------- packages/vcs/tests/E2E/BitbucketTest.php | 26 +- packages/vcs/tests/E2E/ForgejoTest.php | 3 + packages/vcs/tests/E2E/GitHubTest.php | 10 +- packages/vcs/tests/E2E/GitLabTest.php | 3 - packages/vcs/tests/E2E/GiteaBase.php | 2 +- packages/vcs/tests/Unit/Base.php | 72 ++++- packages/vcs/tests/Unit/BitbucketTest.php | 55 ++-- packages/vcs/tests/Unit/GitHubTest.php | 21 +- packages/vcs/tests/Unit/GitLabTest.php | 90 ++---- packages/vcs/tests/Unit/GiteaBase.php | 27 +- 18 files changed, 438 insertions(+), 283 deletions(-) diff --git a/packages/vcs/docs/add-new-vcs-adapter.md b/packages/vcs/docs/add-new-vcs-adapter.md index 30e84346e..17cb0f6dc 100644 --- a/packages/vcs/docs/add-new-vcs-adapter.md +++ b/packages/vcs/docs/add-new-vcs-adapter.md @@ -20,9 +20,12 @@ Below are outlined the most useful files for adding a new VCS adapter: │  │  └── Git.php # Parent class for Git-based adapters │   └── Adapter.php # Parent class for individual adapters └── tests - └── VCS - ├── Adapter/ # Where tests of your new adapter go! - └── Base.php # Parent class that holds all tests + ├── Unit # Payload parsing, on a bare host + │ ├── Base.php # Parent class that holds the webhook contract + │ └── Test.php # Where the offline tests of your new adapter go! + └── E2E # Calls against a running provider + ├── Base.php # Parent class that holds all the rest + └── Test.php # Where the live tests of your new adapter go! ``` ### Extend the adapter 💻 @@ -95,7 +98,13 @@ rather than `getEvent()` — the latter reports only the first event of a batch. ### Testing the adapter 🛠️ -The suite runs in two tiers. Put the webhook payload fixtures and the parsing assertions your adapter answers offline in `tests/Unit/Test.php`, and the calls against a running provider in `tests/E2E/Test.php`. A provider you can self-host joins `docker-compose.yml` as an official image on an offset host port, with a bootstrap that mints an access token into `tests/.tokens`; see the [README](/README.md#tests) for how to run both tiers. +The suite runs in two tiers, and every adapter runs the same tests in each. Each `Base` holds them and each adapter's class declares how its provider differs, so a new adapter is written as declarations rather than as tests of its own: + +1. Extend `Utopia\Tests\Unit\Base` in `tests/Unit/Test.php` and implement its hooks: `createAdapter()` builds the adapter against no provider, `signWebhookPayload()` signs a payload the way the provider does, and `pushPayload()` and `pullRequestPayload()` build webhook payloads shaped the way the provider sends them. Declare `$pullRequestActions` where the provider names its pull request actions differently from GitHub, whose names are the shared vocabulary. +2. Extend `Utopia\Tests\E2E\Base` in `tests/E2E/Test.php` and implement `setupAdapter()` and `anonymousCloneUrl()`. Declare the parts of the contract the provider lacks by overriding the capability flags, such as `$supportsTags` or `$supportsCheckRuns`. The first shared test for each capability then asserts that the adapter refuses with `X() is not supported by `; the tests that need the capability to act on skip. +3. Keep behaviour only this provider has in the adapter's own test class. Anything two providers share belongs in `Base`, behind a declared flag or hook. + +A provider you can self-host joins `docker-compose.yml` as an official image on an offset host port, with a bootstrap that mints an access token into `tests/.tokens`; see the [README](/README.md#tests) for how to run both tiers. ### Tips and tricks 💡 diff --git a/packages/vcs/src/VCS/Adapter/Git.php b/packages/vcs/src/VCS/Adapter/Git.php index f229b3ef2..d40522067 100644 --- a/packages/vcs/src/VCS/Adapter/Git.php +++ b/packages/vcs/src/VCS/Adapter/Git.php @@ -193,6 +193,19 @@ public function getRepositoryPresignedUrl(string $owner, string $repositoryName, throw new Exception('getRepositoryPresignedUrl() is not supported by ' . $this->getName()); } + /** + * Get a repository the installation reaches, by name. + * + * Only GitHub models installations, so the default reports it as + * unsupported. + * + * @return array + */ + public function getInstallationRepository(string $repositoryName): array + { + throw new Exception('getInstallationRepository() is not supported by ' . $this->getName()); + } + /** * Create a check run for a commit. * diff --git a/packages/vcs/src/VCS/Adapter/Git/Bitbucket.php b/packages/vcs/src/VCS/Adapter/Git/Bitbucket.php index 9082c36e9..34d8444db 100644 --- a/packages/vcs/src/VCS/Adapter/Git/Bitbucket.php +++ b/packages/vcs/src/VCS/Adapter/Git/Bitbucket.php @@ -330,11 +330,6 @@ public function hasAccessToAllRepositories(): bool return true; } - public function getInstallationRepository(string $repositoryName): array - { - throw new Exception('getInstallationRepository is not applicable for this adapter'); - } - public function searchRepositories(string $owner, int $page, int $per_page, string $search = ''): array { $url = "/repositories/{$owner}?page={$page}&pagelen={$per_page}"; diff --git a/packages/vcs/src/VCS/Adapter/Git/GitHub.php b/packages/vcs/src/VCS/Adapter/Git/GitHub.php index 70870186d..7177782b1 100644 --- a/packages/vcs/src/VCS/Adapter/Git/GitHub.php +++ b/packages/vcs/src/VCS/Adapter/Git/GitHub.php @@ -115,7 +115,7 @@ public function createRepository(string $owner, string $repositoryName, bool $pr */ public function createPullRequest(string $owner, string $repositoryName, string $title, string $head, string $base, string $body = ''): array { - throw new Exception('Not implemented'); + throw new Exception('createPullRequest() is not supported by ' . $this->getName()); } /** @@ -682,7 +682,15 @@ protected function generateAccessToken(string $privateKey, ?string $appId): void */ public function getUser(string $username): array { - return $this->call(self::METHOD_GET, '/users/' . $username); + $response = $this->call(self::METHOD_GET, '/users/' . rawurlencode($username), ['Authorization' => "Bearer $this->accessToken"]); + + $responseHeaders = $response['headers'] ?? []; + $statusCode = $responseHeaders['status-code'] ?? 0; + if ($statusCode >= 400) { + throw new Exception("Failed to get user: HTTP {$statusCode}", $statusCode); + } + + return $response['body'] ?? []; } /** @@ -1430,11 +1438,11 @@ public function validateWebhookEvent(string $payload, string $signature, string public function createTag(string $owner, string $repositoryName, string $tagName, string $target, string $message = ''): array { - throw new Exception('createTag() is not implemented for GitHub'); + throw new Exception('createTag() is not supported by ' . $this->getName()); } public function getCommitStatuses(string $owner, string $repositoryName, string $commitHash): array { - throw new Exception('getCommitStatuses() is not implemented for GitHub'); + throw new Exception('getCommitStatuses() is not supported by ' . $this->getName()); } } diff --git a/packages/vcs/src/VCS/Adapter/Git/GitLab.php b/packages/vcs/src/VCS/Adapter/Git/GitLab.php index 4eba60187..faa81f1d1 100644 --- a/packages/vcs/src/VCS/Adapter/Git/GitLab.php +++ b/packages/vcs/src/VCS/Adapter/Git/GitLab.php @@ -174,11 +174,12 @@ public function getRepository(string $owner, string $repositoryName): array $projectPath = urlencode("{$ownerPath}/{$repositoryName}"); $url = "/projects/{$projectPath}"; - $response = $this->call(self::METHOD_GET, $url, ['Authorization' => 'Bearer ' . $this->accessToken]); + // GitLab redirects the old path of a renamed or deleted project, which no longer names it + $response = $this->call(self::METHOD_GET, $url, ['Authorization' => 'Bearer ' . $this->accessToken], [], true, false); $responseHeaders = $response['headers'] ?? []; $responseHeadersStatusCode = $responseHeaders['status-code'] ?? 0; - if ($responseHeadersStatusCode >= 400) { + if ($responseHeadersStatusCode !== 200) { throw new RepositoryNotFound('Repository not found'); } @@ -229,11 +230,6 @@ public function hasAccessToAllRepositories(): bool return true; } - public function getInstallationRepository(string $repositoryName): array - { - throw new Exception('getInstallationRepository is not applicable for this adapter'); - } - /** * List namespaces the current user can browse: their personal namespace * plus every group they belong to. GitLab's own /namespaces endpoint @@ -346,8 +342,12 @@ public function getRepositoryName(string $repositoryId): string $responseHeaders = $response['headers'] ?? []; $responseHeadersStatusCode = $responseHeaders['status-code'] ?? 0; + if ($responseHeadersStatusCode === 404) { + throw new RepositoryNotFound("Repository {$repositoryId} not found"); + } + if ($responseHeadersStatusCode >= 400) { - throw new Exception("Repository {$repositoryId} not found"); + throw new Exception("Failed to get repository {$repositoryId}: HTTP {$responseHeadersStatusCode}", $responseHeadersStatusCode); } $responseBody = $response['body'] ?? []; @@ -681,6 +681,9 @@ public function getOwnerName(string $installationId, ?int $repositoryId = null): $response = $this->call(self::METHOD_GET, $url, ['Authorization' => 'Bearer ' . $this->accessToken]); $responseHeaders = $response['headers'] ?? []; $statusCode = $responseHeaders['status-code'] ?? 0; + if ($statusCode === 404) { + throw new RepositoryNotFound("Repository {$repositoryId} not found"); + } if ($statusCode >= 400) { throw new Exception("Failed to get owner name for repository {$repositoryId}: HTTP {$statusCode}", $statusCode); } diff --git a/packages/vcs/src/VCS/Adapter/Git/Gitea.php b/packages/vcs/src/VCS/Adapter/Git/Gitea.php index 8a26b4026..baf29494a 100644 --- a/packages/vcs/src/VCS/Adapter/Git/Gitea.php +++ b/packages/vcs/src/VCS/Adapter/Git/Gitea.php @@ -14,6 +14,12 @@ class Gitea extends Git public const CONTENTS_DIRECTORY = 'dir'; + /** + * Gitea says 'synchronized' for a pushed head where consumers expect + * 'synchronize'; every other action passes through as sent. + */ + private const array PULL_REQUEST_ACTION_MAP = ['synchronized' => 'synchronize']; + protected string $endpoint = 'http://gitea:3000/api/v1'; protected string $accessToken; @@ -224,21 +230,6 @@ public function searchRepositories(string $owner, int $page, int $per_page, stri ]; } - /** - * Get installation repository - * - * Note: Gitea doesn't have GitHub App installations. - * This method is not applicable and throws an exception. - * - * @param string $repositoryName Name of the repository - * @return array - * @throws Exception Always throws as installations don't exist in Gitea - */ - public function getInstallationRepository(string $repositoryName): array - { - throw new Exception('getInstallationRepository is not applicable for this adapter - use getRepository() with owner and repo name instead'); - } - public function getRepository(string $owner, string $repositoryName): array { $url = "/repos/{$owner}/{$repositoryName}"; @@ -1196,6 +1187,7 @@ public function getEvents(string $event, string $payload): array $branchUrl = !empty($repositoryUrl) && !empty($branch) ? $repositoryUrl . '/src/branch/' . $branch : ''; $pullRequestNumber = $payload['number'] ?? ''; $action = $payload['action'] ?? ''; + $action = self::PULL_REQUEST_ACTION_MAP[$action] ?? $action; $owner = $payloadRepositoryOwner['login'] ?? ''; $authorUrl = $payloadSender['html_url'] ?? ''; $authorAvatarUrl = $payloadPullRequestUser['avatar_url'] ?? ''; diff --git a/packages/vcs/src/VCS/Adapter/Git/Gogs.php b/packages/vcs/src/VCS/Adapter/Git/Gogs.php index 7ece69327..e1ce72ee9 100644 --- a/packages/vcs/src/VCS/Adapter/Git/Gogs.php +++ b/packages/vcs/src/VCS/Adapter/Git/Gogs.php @@ -447,7 +447,7 @@ private function exec(string $command): string #[\Override] public function listRepositoryLanguages(string $owner, string $repositoryName): array { - throw new Exception('Listing repository languages is not supported by Gogs'); + throw new Exception('listRepositoryLanguages() is not supported by ' . $this->getName()); } /** @@ -492,7 +492,7 @@ public function createTag(string $owner, string $repositoryName, string $tagName #[\Override] public function createPullRequest(string $owner, string $repositoryName, string $title, string $head, string $base, string $body = ''): array { - throw new Exception('Pull request API is not supported by Gogs'); + throw new Exception('createPullRequest() is not supported by ' . $this->getName()); } /** @@ -503,7 +503,7 @@ public function createPullRequest(string $owner, string $repositoryName, string #[\Override] public function getPullRequest(string $owner, string $repositoryName, int $pullRequestNumber): array { - throw new Exception('Pull request API is not supported by Gogs'); + throw new Exception('getPullRequest() is not supported by ' . $this->getName()); } /** @@ -514,7 +514,7 @@ public function getPullRequest(string $owner, string $repositoryName, int $pullR #[\Override] public function getPullRequestFromBranch(string $owner, string $repositoryName, string $branch): array { - throw new Exception('Pull request API is not supported by Gogs'); + throw new Exception('getPullRequestFromBranch() is not supported by ' . $this->getName()); } /** @@ -525,7 +525,7 @@ public function getPullRequestFromBranch(string $owner, string $repositoryName, #[\Override] public function getPullRequestFiles(string $owner, string $repositoryName, int $pullRequestNumber): array { - throw new Exception('Pull request API is not supported by Gogs'); + throw new Exception('getPullRequestFiles() is not supported by ' . $this->getName()); } /** @@ -536,7 +536,7 @@ public function getPullRequestFiles(string $owner, string $repositoryName, int $ #[\Override] public function updateCommitStatus(string $repositoryName, string $commitHash, string $owner, string $state, string $description = '', string $target_url = '', string $context = ''): void { - throw new Exception('Commit status API is not supported by Gogs'); + throw new Exception('updateCommitStatus() is not supported by ' . $this->getName()); } /** @@ -549,7 +549,7 @@ public function updateCommitStatus(string $repositoryName, string $commitHash, s #[\Override] public function getCommitStatuses(string $owner, string $repositoryName, string $commitHash): array { - throw new Exception('Commit status API is not supported by Gogs'); + throw new Exception('getCommitStatuses() is not supported by ' . $this->getName()); } /** diff --git a/packages/vcs/tests/E2E/Base.php b/packages/vcs/tests/E2E/Base.php index 3ed630e07..8dfd33995 100644 --- a/packages/vcs/tests/E2E/Base.php +++ b/packages/vcs/tests/E2E/Base.php @@ -19,7 +19,9 @@ abstract class Base extends TestCase protected static string $defaultBranch = 'main'; /** - * Username of an account that exists on the instance under test. + * Handle of an account that exists on the instance under test. Where the + * provider resolves an owner from a repository, it is also the account the + * token belongs to, which getOwnerName() reports when given no repository. */ protected static string $existingUser = 'root'; @@ -48,8 +50,8 @@ abstract class Base extends TestCase /** * Actions the provider may report for a newly opened pull request. Gitea - * follows the opened event with a synchronized one for the head it just - * pushed, and the catcher only keeps the last delivery. + * and GitLab may follow the opened event with a synchronize one for the + * head just pushed, and the catcher only keeps the last delivery. * * @var array */ @@ -74,15 +76,9 @@ abstract class Base extends TestCase protected static bool $supportsInstallationRepository = true; /** - * Exception the provider raises for a repository id that does not exist. - * - * @var class-string<\Throwable> - */ - protected static string $repositoryNotFoundException = RepositoryNotFound::class; - - /** - * Parts of the contract a provider may not offer at all. Each one skips the - * tests that need it, instead of every adapter overriding them to say so. + * Parts of the contract a provider may not offer at all. Where the adapter + * documents a refusal, the test asserts it; the rest skip what they cannot + * reach. */ protected static bool $supportsPullRequestCreation = true; @@ -94,10 +90,21 @@ abstract class Base extends TestCase protected static bool $supportsTags = true; - protected static bool $supportsUserLookup = true; + /** + * Whether getUser() takes a handle. Bitbucket looks accounts up by UUID or + * Atlassian account id, so a handle resolves nothing there. + */ + protected static bool $resolvesUsersByHandle = true; protected static bool $supportsRepositoryLanguages = true; + /** + * Whether the provider works out the languages a repository holds. Bitbucket + * reports the single language a repository was labelled with instead, so it + * answers the call without ever describing the files. + */ + protected static bool $detectsRepositoryLanguages = true; + protected static bool $supportsWebhookDelivery = true; protected static bool $resolvesOwnerFromRepositoryId = true; @@ -118,16 +125,11 @@ abstract class Base extends TestCase protected static bool $computesLanguagesAsynchronously = false; /** - * Host the provider serves commit author avatars from. + * Fragment of the URL the provider serves commit author avatars from: a + * host, or a path where the provider serves them itself. */ protected static string $avatarDomain = ''; - /** - * Whether a repository is gone as soon as delete returns. GitLab schedules - * it instead. - */ - protected static bool $deletesRepositoriesSynchronously = true; - /** * Whether a new repository starts with no commits. The Gogs adapter creates * one with an initial commit, so it never has an empty repository. @@ -136,7 +138,7 @@ abstract class Base extends TestCase /** * Whether the provider links the commit author back to an account. GitLab - * reports neither, Gitea an avatar but no profile url. + * reports neither; Gitea an avatar but no profile url, which Forgejo adds. */ protected static bool $reportsCommitAuthorAvatar = true; @@ -205,7 +207,7 @@ protected function ownerPath(): string } /** - * Owner of a repository, as GitHub and Gitea report it. GitLab overrides this. + * Owner of a repository, as GitHub and Gitea report it. GitLab and Bitbucket override this. * * @param array $repository */ @@ -266,10 +268,14 @@ protected function assertCommitAuthorLinks(array $commit): void { if (static::$reportsCommitAuthorAvatar) { $this->assertNotEmpty($commit['commitAuthorAvatar']); + } else { + $this->assertSame('', $commit['commitAuthorAvatar']); } if (static::$reportsCommitAuthorUrl) { $this->assertNotEmpty($commit['commitAuthorUrl']); + } else { + $this->assertSame('', $commit['commitAuthorUrl']); } } @@ -280,6 +286,20 @@ protected function skipUnlessSupported(bool $supported, string $capability): voi } } + /** + * A provider that does not offer a capability still has to say so. Where + * the adapter documents a refusal, assert it rather than skipping, so + * declaring a capability unsupported narrows a test instead of dropping it. + * The wording is what tells a refusal from a failed call: the repository + * the call names never exists, so a 404 must not pass for one. + */ + protected function assertRefused(callable $call): void + { + $this->expectException(Exception::class); + $this->expectExceptionMessageMatches('/^\w+\(\) is not supported by /'); + $call(); + } + protected function assertEventually(callable $probe, int $timeoutMs = 15000, int $waitMs = 500): void { $start = microtime(true) * 1000; @@ -471,7 +491,7 @@ public function testGetRepository(): void } } - public function testGetDeletedRepositoryFails(): void + public function testGetNonExistingRepositoryFails(): void { $this->expectException(RepositoryNotFound::class); $this->vcsAdapter->getRepository(static::$owner, 'non-existing-repository-' . uniqid()); @@ -479,7 +499,7 @@ public function testGetDeletedRepositoryFails(): void public function testGetRepositoryWithNonExistingOwner(): void { - $this->expectException(Exception::class); + $this->expectException(RepositoryNotFound::class); $this->vcsAdapter->getRepository('non-existing-owner-' . uniqid(), 'non-existing-repo'); } @@ -532,7 +552,7 @@ public function testGetRepositoryName(): void public function testGetRepositoryNameWithInvalidId(): void { - $this->expectException(Exception::class); + $this->expectException(RepositoryNotFound::class); $this->vcsAdapter->getRepositoryName('99999999'); } @@ -600,7 +620,7 @@ public function testGetRepositoryContent(): void $this->assertIsString($result['sha']); $this->assertArrayHasKey('size', $result); $this->assertSame($fileContent, $result['content']); - $this->assertGreaterThan(0, $result['size']); + $this->assertSame(\strlen($fileContent), $result['size']); } finally { $this->discardRepositories($repositoryName); } @@ -685,7 +705,11 @@ public function testListRepositoryContentsNonExistingPath(): void public function testListRepositoryLanguages(): void { - $this->skipUnlessSupported(static::$supportsRepositoryLanguages, 'repository languages'); + if (!static::$supportsRepositoryLanguages) { + $this->assertRefused(fn(): array => $this->vcsAdapter->listRepositoryLanguages(static::$owner, 'unsupported-languages-' . uniqid())); + + return; + } $repositoryName = 'test-list-repository-languages-' . uniqid(); $this->vcsAdapter->createRepository(static::$owner, $repositoryName, false); @@ -694,6 +718,13 @@ public function testListRepositoryLanguages(): void $this->vcsAdapter->createFile(static::$owner, $repositoryName, 'main.php', 'vcsAdapter->createFile(static::$owner, $repositoryName, 'script.js', 'console.log("test");'); + if (!static::$detectsRepositoryLanguages) { + // Nothing labelled the repository, and the files are never inspected + $this->assertSame([], $this->vcsAdapter->listRepositoryLanguages(static::$owner, $repositoryName)); + + return; + } + $languages = []; try { $this->assertEventually(function () use (&$languages, $repositoryName): void { @@ -718,6 +749,8 @@ public function testListRepositoryLanguagesEmptyRepo(): void { $this->skipUnlessSupported(static::$supportsRepositoryLanguages, 'repository languages'); + // A repository with nothing in it has no languages to report, whether or + // not the provider works them out from the files $repositoryName = 'test-list-repository-languages-empty-' . uniqid(); $this->vcsAdapter->createRepository(static::$owner, $repositoryName, false); @@ -756,15 +789,14 @@ public function testListBranches(): void public function testListBranchesEmptyRepository(): void { - $this->skipUnlessSupported(static::$createsEmptyRepositories, 'repositories without an initial commit'); - $repositoryName = 'test-list-branches-empty-' . uniqid(); $this->vcsAdapter->createRepository(static::$owner, $repositoryName, false); try { - $branches = $this->vcsAdapter->listBranches(static::$owner, $repositoryName); - - $this->assertEmpty($branches); + $this->assertSame( + static::$createsEmptyRepositories ? [] : [static::$defaultBranch], + $this->vcsAdapter->listBranches(static::$owner, $repositoryName), + ); } finally { $this->discardRepositories($repositoryName); } @@ -908,7 +940,16 @@ public function testGetLatestCommitWithInvalidBranch(): void public function testUpdateCommitStatus(): void { - $this->skipUnlessSupported(static::$supportsCommitStatuses, 'commit statuses'); + if (!static::$supportsCommitStatuses) { + $this->assertRefused(fn() => $this->vcsAdapter->updateCommitStatus( + 'unsupported-commit-statuses-' . uniqid(), + 'abc123', + static::$owner, + 'success', + )); + + return; + } $repositoryName = 'test-update-commit-status-' . uniqid(); $this->vcsAdapter->createRepository(static::$owner, $repositoryName, false); @@ -1056,23 +1097,11 @@ public function testGenerateCloneCommandWithInvalidRepository(): void public function testGetOwnerNameWithoutRepositoryId(): void { - $this->skipUnlessSupported(static::$resolvesOwnerFromRepositoryId, 'resolving an owner from a repository id'); - - $this->assertSame(static::$existingUser, $this->vcsAdapter->getOwnerName('')); - } - - public function testGetOwnerNameWithZeroRepositoryId(): void - { - $this->skipUnlessSupported(static::$resolvesOwnerFromRepositoryId, 'resolving an owner from a repository id'); - - $this->assertSame(static::$existingUser, $this->vcsAdapter->getOwnerName('', 0)); - } - - public function testGetOwnerNameWithNullRepositoryId(): void - { - $this->skipUnlessSupported(static::$resolvesOwnerFromRepositoryId, 'resolving an owner from a repository id'); + $expected = static::$resolvesOwnerFromRepositoryId ? static::$existingUser : $this->ownerPath(); - $this->assertSame(static::$existingUser, $this->vcsAdapter->getOwnerName('')); + // No id at all and a zero id both mean "no repository" + $this->assertSame($expected, $this->vcsAdapter->getOwnerName(static::$installationId)); + $this->assertSame($expected, $this->vcsAdapter->getOwnerName(static::$installationId, 0)); } public function testGetOwnerName(): void @@ -1095,44 +1124,46 @@ public function testGetOwnerName(): void public function testCreateRepositoryWithInvalidName(): void { - $this->skipUnlessSupported(static::$rejectsInvalidRepositoryNames, 'rejecting invalid repository names'); + $uniq = uniqid(); + $invalidName = 'invalid name with spaces ' . $uniq; + + if (!static::$rejectsInvalidRepositoryNames) { + // GitHub replaces the spaces with hyphens rather than refusing the name + $normalizedName = 'invalid-name-with-spaces-' . $uniq; + $created = $this->vcsAdapter->createRepository(static::$owner, $invalidName, false); + + try { + $this->assertSame($normalizedName, $created['name']); + $this->assertSame($normalizedName, $this->vcsAdapter->getRepository(static::$owner, $normalizedName)['name']); + } finally { + $this->discardRepositories($normalizedName); + } + + return; + } $this->expectException(Exception::class); - $this->vcsAdapter->createRepository(static::$owner, 'invalid name with spaces', false); + $this->vcsAdapter->createRepository(static::$owner, $invalidName, false); } public function testGenerateCloneCommandWithTag(): void { - $this->skipUnlessSupported(static::$supportsTags, 'creating tags'); - - $repositoryName = 'test-clone-tag-' . uniqid(); - $this->vcsAdapter->createRepository(static::$owner, $repositoryName, false); - $directory = '/tmp/test-clone-tag-' . uniqid(); - - try { - $this->vcsAdapter->createFile(static::$owner, $repositoryName, 'README.md', '# Test Tag'); - $commitHash = $this->getLatestCommitEventually($repositoryName)['commitHash']; - - $this->vcsAdapter->createTag(static::$owner, $repositoryName, 'v1.0.0', $commitHash, 'Release v1.0.0'); - - $command = $this->vcsAdapter->generateCloneCommand( - static::$owner, - $repositoryName, - 'v1.0.0', - Git::CLONE_TYPE_TAG, - $directory, - '/', - ); + // The command is built, never run, so the tag does not have to exist + $command = $this->vcsAdapter->generateCloneCommand( + static::$owner, + 'test-clone-tag-' . uniqid(), + 'v1.0.0', + Git::CLONE_TYPE_TAG, + '/tmp/test-clone-tag-' . uniqid(), + '/', + ); - $this->assertStringContainsString('git init', $command); - $this->assertStringContainsString('git remote add origin', $command); - $this->assertStringContainsString('git config core.sparseCheckout true', $command); - $this->assertStringContainsString('refs/tags', $command); - $this->assertStringContainsString('v1.0.0', $command); - $this->assertStringContainsString('git checkout FETCH_HEAD', $command); - } finally { - $this->discardRepositories($repositoryName); - } + $this->assertStringContainsString('git init', $command); + $this->assertStringContainsString('git remote add origin', $command); + $this->assertStringContainsString('git config core.sparseCheckout true', $command); + $this->assertStringContainsString('refs/tags', $command); + $this->assertStringContainsString('v1.0.0', $command); + $this->assertStringContainsString('git checkout FETCH_HEAD', $command); } public function testSearchRepositoriesMatchesName(): void @@ -1190,7 +1221,17 @@ public function testSearchRepositories(): void public function testGetPullRequest(): void { - $this->skipUnlessSupported(static::$supportsPullRequestCreation, 'creating pull requests'); + if (!static::$supportsPullRequestCreation) { + $this->assertRefused(fn(): array => $this->vcsAdapter->createPullRequest( + static::$owner, + 'unsupported-pull-requests-' . uniqid(), + 'Test PR', + 'feature-branch', + static::$defaultBranch, + )); + + return; + } $repositoryName = 'test-get-pull-request-' . uniqid(); $this->vcsAdapter->createRepository(static::$owner, $repositoryName, false); @@ -1264,7 +1305,15 @@ public function testGetPullRequestFiles(): void public function testGetPullRequestWithInvalidNumber(): void { - $this->skipUnlessSupported(static::$supportsPullRequestLookup, 'looking up pull requests'); + if (!static::$supportsPullRequestLookup) { + $this->assertRefused(fn(): array => $this->vcsAdapter->getPullRequest( + static::$owner, + 'unsupported-pull-request-lookup-' . uniqid(), + 99999, + )); + + return; + } $repositoryName = 'test-get-pull-request-invalid-' . uniqid(); $this->vcsAdapter->createRepository(static::$owner, $repositoryName, false); @@ -1453,21 +1502,19 @@ public function testGetCommentInvalidId(): void public function testGetUser(): void { - $this->skipUnlessSupported(static::$supportsUserLookup, 'looking up users'); + $this->skipUnlessSupported(static::$resolvesUsersByHandle, 'resolving users by handle'); $result = $this->vcsAdapter->getUser(static::$existingUser); $this->assertArrayHasKey('id', $result); $this->assertNotEmpty($result['id']); - // GitLab reports the handle as 'username', Gitea and its forks as 'login' + // GitLab reports the handle as 'username', the others as 'login' $this->assertArrayHasKey(static::$userHandleField, $result); $this->assertSame(static::$existingUser, $result[static::$userHandleField]); } public function testGetUserWithInvalidUsername(): void { - $this->skipUnlessSupported(static::$supportsUserLookup, 'looking up users'); - $this->expectException(Exception::class); $this->vcsAdapter->getUser('non-existent-user-' . uniqid()); } @@ -1542,7 +1589,9 @@ public function testGetRepositoryPresignedUrl(): void public function testGetRepositoryPresignedUrlWithInvalidFormat(): void { + // The repository never exists, so only the format check may throw $this->expectException(Exception::class); + $this->expectExceptionMessageMatches('/^Invalid archive format/'); $this->vcsAdapter->getRepositoryPresignedUrl(static::$owner, 'some-repo', static::$defaultBranch, 'invalid'); } @@ -1554,8 +1603,7 @@ public function testHasAccessToAllRepositories(): void public function testGetInstallationRepository(): void { if (!static::$supportsInstallationRepository) { - $this->expectException(Exception::class); - $this->vcsAdapter->getInstallationRepository('any-repo-name'); + $this->assertRefused(fn(): array => $this->vcsAdapter->getInstallationRepository('any-repo-name')); return; } @@ -1586,7 +1634,7 @@ public function testGetOwnerNameWithInvalidRepositoryId(): void return; } - $this->expectException(static::$repositoryNotFoundException); + $this->expectException(RepositoryNotFound::class); $this->vcsAdapter->getOwnerName('', 999999999); } @@ -1696,7 +1744,16 @@ public function testGetRepositoryTreeWithSlashInBranchName(): void public function testCreateTag(): void { - $this->skipUnlessSupported(static::$supportsTags, 'creating tags'); + if (!static::$supportsTags) { + $this->assertRefused(fn(): array => $this->vcsAdapter->createTag( + static::$owner, + 'unsupported-tags-' . uniqid(), + 'v1.0.0', + 'abc123', + )); + + return; + } $repositoryName = 'test-create-tag-' . uniqid(); $this->vcsAdapter->createRepository(static::$owner, $repositoryName, false); @@ -1745,14 +1802,13 @@ public function testSearchRepositoriesPagination(): void public function testListTagsCommitlessRepository(): void { - $this->skipUnlessSupported(static::$createsEmptyRepositories, 'repositories without an initial commit'); - $repositoryName = 'test-list-tags-commitless-' . uniqid(); $this->vcsAdapter->createRepository(static::$owner, $repositoryName, false); try { - // No commits at all, which some providers answer differently from - // a repository that simply has no tags + // A repository with no commits, which some providers answer + // differently from one that has commits but no tags; the initial + // commit Gogs starts with carries none either $this->assertSame([], $this->vcsAdapter->listTags(static::$owner, $repositoryName)); } finally { $this->discardRepositories($repositoryName); @@ -1761,7 +1817,15 @@ public function testListTagsCommitlessRepository(): void public function testGetCommitStatuses(): void { - $this->skipUnlessSupported(static::$supportsCommitStatusLookup, 'reading commit statuses'); + if (!static::$supportsCommitStatusLookup) { + $this->assertRefused(fn(): array => $this->vcsAdapter->getCommitStatuses( + static::$owner, + 'unsupported-commit-status-lookup-' . uniqid(), + 'abc123', + )); + + return; + } $repositoryName = 'test-get-commit-statuses-' . uniqid(); $this->vcsAdapter->createRepository(static::$owner, $repositoryName, false); @@ -1806,7 +1870,16 @@ public function testGetCommitStatusesEmptyForNewCommit(): void public function testCreateCheckRun(): void { - $this->skipUnlessSupported(static::$supportsCheckRuns, 'check runs'); + if (!static::$supportsCheckRuns) { + $this->assertRefused(fn(): array => $this->vcsAdapter->createCheckRun( + owner: static::$owner, + repositoryName: 'unsupported-check-runs-' . uniqid(), + headSha: 'abc123', + name: 'ci/build', + )); + + return; + } $repositoryName = 'test-create-check-run-' . uniqid(); $this->vcsAdapter->createRepository(static::$owner, $repositoryName, false); @@ -1845,7 +1918,7 @@ public function testCreateCheckRun(): void $this->assertNotEmpty($fetched['url']); $this->assertNotEmpty($fetched['html_url']); } finally { - $this->vcsAdapter->deleteRepository(static::$owner, $repositoryName); + $this->discardRepositories($repositoryName); } } public function testCreateCheckRunWithInvalidRepository(): void @@ -1871,7 +1944,7 @@ public function testGetCheckRunWithInvalidId(): void $this->expectException(\Exception::class); $this->vcsAdapter->getCheckRun(static::$owner, $repositoryName, '999999999'); } finally { - $this->vcsAdapter->deleteRepository(static::$owner, $repositoryName); + $this->discardRepositories($repositoryName); } } public function testCreateTwoCheckRunsOnSameCommit(): void @@ -1911,7 +1984,7 @@ public function testCreateTwoCheckRunsOnSameCommit(): void $this->assertEquals('ci/build', $first['name']); $this->assertEquals('ci/build', $second['name']); } finally { - $this->vcsAdapter->deleteRepository(static::$owner, $repositoryName); + $this->discardRepositories($repositoryName); } } public function testCreateCheckRunsWithSameNameOnDifferentCommits(): void @@ -1954,7 +2027,7 @@ public function testCreateCheckRunsWithSameNameOnDifferentCommits(): void $this->assertEquals('ci/build', $first['name']); $this->assertEquals('ci/build', $second['name']); } finally { - $this->vcsAdapter->deleteRepository(static::$owner, $repositoryName); + $this->discardRepositories($repositoryName); } } public function testCreateCheckRunCompleted(): void @@ -1992,7 +2065,7 @@ public function testCreateCheckRunCompleted(): void $this->assertEquals('Build passed', $checkRun['output']['title']); $this->assertEquals('All checks passed successfully.', $checkRun['output']['summary']); } finally { - $this->vcsAdapter->deleteRepository(static::$owner, $repositoryName); + $this->discardRepositories($repositoryName); } } public function testUpdateCheckRun(): void @@ -2034,7 +2107,7 @@ public function testUpdateCheckRun(): void $this->assertEquals('completed', $updated['status']); $this->assertEquals('neutral', $updated['conclusion']); } finally { - $this->vcsAdapter->deleteRepository(static::$owner, $repositoryName); + $this->discardRepositories($repositoryName); } } public function testUpdateCheckRunWithInvalidRepository(): void @@ -2065,7 +2138,7 @@ public function testUpdateCheckRunWithInvalidId(): void conclusion: 'success', ); } finally { - $this->vcsAdapter->deleteRepository(static::$owner, $repositoryName); + $this->discardRepositories($repositoryName); } } public function testUpdateCheckRunWithMissingConclusion(): void @@ -2097,13 +2170,17 @@ public function testUpdateCheckRunWithMissingConclusion(): void status: 'completed', ); } finally { - $this->vcsAdapter->deleteRepository(static::$owner, $repositoryName); + $this->discardRepositories($repositoryName); } } public function testListNamespaces(): void { - $this->skipUnlessSupported(static::$supportsNamespaceListing, 'listing namespaces'); + if (!static::$supportsNamespaceListing) { + $this->assertRefused(fn(): array => $this->vcsAdapter->listNamespaces(1, 20)); + + return; + } $result = $this->vcsAdapter->listNamespaces(1, 20); @@ -2156,7 +2233,7 @@ public function testListRepositoryContentsRootSentinels(): void $this->assertEquals(array_column($empty, 'name'), array_column($dotSlash, 'name')); $this->assertEquals(array_column($empty, 'name'), array_column($repeatedDotSlash, 'name')); } finally { - $this->vcsAdapter->deleteRepository(static::$owner, $repositoryName); + $this->discardRepositories($repositoryName); } } public function testGetRepositoryContentRootSentinelPrefix(): void @@ -2174,7 +2251,7 @@ public function testGetRepositoryContentRootSentinelPrefix(): void $this->assertEquals($direct['content'], $prefixed['content']); $this->assertEquals($direct['content'], $repeatedPrefix['content']); } finally { - $this->vcsAdapter->deleteRepository(static::$owner, $repositoryName); + $this->discardRepositories($repositoryName); } } public function testListRepositoryContentsMalformedNestedPath(): void @@ -2193,7 +2270,7 @@ public function testListRepositoryContentsMalformedNestedPath(): void $this->assertEquals(array_column($clean, 'name'), array_column($embeddedDot, 'name')); $this->assertEquals(array_column($clean, 'name'), array_column($doubleSlash, 'name')); } finally { - $this->vcsAdapter->deleteRepository(static::$owner, $repositoryName); + $this->discardRepositories($repositoryName); } } @@ -2232,8 +2309,6 @@ public function testGetRepositoryContentReportsBlobSha(): void public function testGetCommitAuthorAvatar(): void { - $this->skipUnlessSupported(static::$reportsCommitAuthorAvatar, 'commit author avatars'); - $repositoryName = 'test-get-commit-avatar-' . uniqid(); $this->vcsAdapter->createRepository(static::$owner, $repositoryName, false); @@ -2243,16 +2318,21 @@ public function testGetCommitAuthorAvatar(): void $commit = $this->vcsAdapter->getCommit(static::$owner, $repositoryName, $commitHash); + if (!static::$reportsCommitAuthorAvatar) { + $this->assertSame('', $commit['commitAuthorAvatar']); + + return; + } + $this->assertNotEmpty($commit['commitAuthorAvatar']); $this->assertStringContainsString(static::$avatarDomain, (string) $commit['commitAuthorAvatar']); } finally { - $this->vcsAdapter->deleteRepository(static::$owner, $repositoryName); + $this->discardRepositories($repositoryName); } } + public function testGetRepositoryAfterDeleteFails(): void { - $this->skipUnlessSupported(static::$deletesRepositoriesSynchronously, 'deleting a repository straight away'); - $repositoryName = 'test-get-deleted-repository-' . uniqid(); $this->vcsAdapter->createRepository(static::$owner, $repositoryName, false); $this->vcsAdapter->deleteRepository(static::$owner, $repositoryName); diff --git a/packages/vcs/tests/E2E/BitbucketTest.php b/packages/vcs/tests/E2E/BitbucketTest.php index fc9a52d1e..ec153d1a2 100644 --- a/packages/vcs/tests/E2E/BitbucketTest.php +++ b/packages/vcs/tests/E2E/BitbucketTest.php @@ -17,7 +17,10 @@ final class BitbucketTest extends Base protected static string $defaultBranch = 'master'; protected static bool $supportsInstallationRepository = false; - protected static bool $supportsRepositoryLanguages = false; + + // A repository reports the one language it was labelled with, not the + // languages of the files it holds + protected static bool $detectsRepositoryLanguages = false; // Bitbucket has no repository to resolve an owner from; getOwnerName() // reports the account the token belongs to @@ -27,7 +30,7 @@ final class BitbucketTest extends Base protected static bool $reportsNamespaceKinds = false; // Accounts are looked up by uuid, not by handle - protected static bool $supportsUserLookup = false; + protected static bool $resolvesUsersByHandle = false; // Bitbucket Cloud can't reach a local test catcher protected static bool $supportsWebhookDelivery = false; @@ -75,23 +78,4 @@ protected function ownerOf(array $repository): string return (string) $repository['workspace']['slug']; } - - /** - * @return array - */ - - /** - * @return array - */ - - /** - * Bitbucket only names the author in a raw "Name " string; a commit - * linked to an account is named by the account instead. - */ - - /** - * Bitbucket batches every ref a push touched into one delivery, and the - * shared event shape describes a branch push, so tags are left out. - */ - } diff --git a/packages/vcs/tests/E2E/ForgejoTest.php b/packages/vcs/tests/E2E/ForgejoTest.php index c6eeef3ca..8fd918d8b 100644 --- a/packages/vcs/tests/E2E/ForgejoTest.php +++ b/packages/vcs/tests/E2E/ForgejoTest.php @@ -14,6 +14,9 @@ final class ForgejoTest extends GiteaBase protected static string $owner = ''; protected static string $avatarDomain = '/avatars/'; + // Forgejo's API user carries html_url, which Gitea 1.21's does not + protected static bool $reportsCommitAuthorUrl = true; + protected function setupAdapter(): void { $adapter = new Forgejo(new Cache(new None())); diff --git a/packages/vcs/tests/E2E/GitHubTest.php b/packages/vcs/tests/E2E/GitHubTest.php index 27b7e8f0d..4b14aa725 100644 --- a/packages/vcs/tests/E2E/GitHubTest.php +++ b/packages/vcs/tests/E2E/GitHubTest.php @@ -12,14 +12,15 @@ final class GitHubTest extends Base { protected static string $owner = ''; - protected static string $defaultBranch = 'main'; + protected static string $existingUser = ''; + protected static string $installationId = ''; + protected static string $userHandleField = 'login'; protected static string $avatarDomain = 'githubusercontent.com'; protected static bool $supportsPullRequestCreation = false; protected static bool $supportsNamespaceListing = false; protected static bool $supportsCommitStatusLookup = false; protected static bool $supportsTags = false; - protected static bool $supportsUserLookup = false; protected static bool $computesLanguagesAsynchronously = true; protected static bool $supportsWebhookDelivery = false; protected static bool $resolvesOwnerFromRepositoryId = false; @@ -53,6 +54,9 @@ protected function setupAdapter(): void self::$owner = $adapter->getOwnerName(self::$installationId); } + // The account the app is installed on is the one user known to exist + self::$existingUser = self::$owner; + $this->vcsAdapter = $adapter; } @@ -90,7 +94,7 @@ public function testListBranchesPagination(): void $noMatch = $adapter->listBranches(self::$owner, $repositoryName, 100, 1, 'xyz'); $this->assertEmpty($noMatch); } finally { - $this->vcsAdapter->deleteRepository(self::$owner, $repositoryName); + $this->discardRepositories($repositoryName); } } } diff --git a/packages/vcs/tests/E2E/GitLabTest.php b/packages/vcs/tests/E2E/GitLabTest.php index 239429c06..4a5380f4b 100644 --- a/packages/vcs/tests/E2E/GitLabTest.php +++ b/packages/vcs/tests/E2E/GitLabTest.php @@ -12,7 +12,6 @@ final class GitLabTest extends Base { protected static string $owner = ''; - protected static string $defaultBranch = 'main'; protected static string $openPullRequestState = 'opened'; /** @@ -26,8 +25,6 @@ final class GitLabTest extends Base protected static string $presignedTarballFragment = '/repository/archive.tar.gz?access_token='; protected static string $presignedZipballFragment = '/repository/archive.zip?access_token='; - protected static string $repositoryNotFoundException = \Exception::class; - protected static bool $deletesRepositoriesSynchronously = false; protected static bool $supportsCheckRuns = false; protected static bool $supportsInstallationRepository = false; protected static bool $reportsCommitAuthorAvatar = false; diff --git a/packages/vcs/tests/E2E/GiteaBase.php b/packages/vcs/tests/E2E/GiteaBase.php index 4482e6b1e..6c3fdd866 100644 --- a/packages/vcs/tests/E2E/GiteaBase.php +++ b/packages/vcs/tests/E2E/GiteaBase.php @@ -14,7 +14,7 @@ abstract class GiteaBase extends Base protected static string $userHandleField = 'login'; /** @var array */ - protected static array $pullRequestOpenedActions = ['opened', 'synchronized']; + protected static array $pullRequestOpenedActions = ['opened', 'synchronize']; protected static string $presignedTarballFragment = '.tar.gz?token='; protected static string $presignedZipballFragment = '.zip?token='; diff --git a/packages/vcs/tests/Unit/Base.php b/packages/vcs/tests/Unit/Base.php index bcf780f71..3232b76be 100644 --- a/packages/vcs/tests/Unit/Base.php +++ b/packages/vcs/tests/Unit/Base.php @@ -56,6 +56,19 @@ abstract class Base extends TestCase protected static string $pullRequestEventName = 'pull_request'; + /** + * Actions the provider sends for a pull request, each with the shared + * vocabulary it normalizes to. GitHub's names are that vocabulary. + * + * @var array + */ + protected static array $pullRequestActions = [ + 'opened' => 'opened', + 'reopened' => 'reopened', + 'synchronize' => 'synchronize', + 'closed' => 'closed', + ]; + /** * Headers the provider sends its webhook event type and signature under. */ @@ -87,14 +100,25 @@ abstract protected function signWebhookPayload(string $payload, string $secret): * @param array $added * @param array $removed * @param array $modified + * @param array $olderCommits Hashes listed before the head commit, each with its own message and author */ - abstract protected function pushPayload(string $branch, array $added = [], array $removed = [], array $modified = [], bool $created = false, bool $deleted = false): string; + abstract protected function pushPayload(string $branch, array $added = [], array $removed = [], array $modified = [], bool $created = false, bool $deleted = false, array $olderCommits = []): string; /** * Build a pull request payload shaped the way this provider sends one, - * opening EVENT_HEAD_BRANCH against the default branch. + * opening EVENT_HEAD_BRANCH against the default branch. The action is the + * provider's own name for it, and defaults to its opened one. */ - abstract protected function pullRequestPayload(bool $external = false): string; + abstract protected function pullRequestPayload(bool $external = false, string $action = 'opened'): string; + + /** + * Event a pull request action is delivered under. Bitbucket names the + * action in the event rather than the payload. + */ + protected function pullRequestEventFor(string $action): string + { + return static::$pullRequestEventName; + } protected function setUp(): void { @@ -152,6 +176,27 @@ public function testGetEventPush(): void ); } + /** + * A push lists every commit it carried; the event describes the head, not + * the first one listed. + */ + public function testGetEventPushReportsHeadCommit(): void + { + $events = $this->vcsAdapter->getEvents( + static::$pushEventName, + $this->pushPayload(static::$defaultBranch, olderCommits: ['aaa111', 'bbb222']), + ); + $this->assertCount(1, $events); + $result = $events[0]; + + $this->assertSame(self::EVENT_COMMIT_HASH, $result['commitHash']); + $this->assertSame(self::EVENT_COMMIT_MESSAGE, $result['headCommitMessage']); + $this->assertSame(self::EVENT_AUTHOR_NAME, $result['headCommitAuthorName']); + $this->assertSame(self::EVENT_AUTHOR_EMAIL, $result['headCommitAuthorEmail']); + // Providers shape the commit url differently, but each ends it with the hash + $this->assertStringEndsWith('/' . self::EVENT_COMMIT_HASH, $result['headCommitUrl']); + } + public function testGetEventPushDetectsBranchCreated(): void { $events = $this->vcsAdapter->getEvents( @@ -203,6 +248,27 @@ public function testGetEventPullRequestDetectsExternal(): void $this->assertTrue($result['external']); } + /** + * Every pull request action normalizes to the shared vocabulary, and each + * provider sends the three consumers act on: opened, synchronize, closed. + */ + public function testGetEventPullRequestNormalizesAction(): void + { + $vocabulary = ['opened', 'reopened', 'synchronize', 'closed']; + + $this->assertSame([], array_diff(static::$pullRequestActions, $vocabulary)); + $this->assertSame([], array_diff(['opened', 'synchronize', 'closed'], static::$pullRequestActions)); + + foreach (static::$pullRequestActions as $native => $normalized) { + $events = $this->vcsAdapter->getEvents( + $this->pullRequestEventFor($native), + $this->pullRequestPayload(action: $native), + ); + $this->assertCount(1, $events, "No event for the '{$native}' action"); + $this->assertSame($normalized, $events[0]['action'], "The '{$native}' action did not normalize to '{$normalized}'"); + } + } + public function testGetEventInvalidPayload(): void { $this->expectException(Exception::class); diff --git a/packages/vcs/tests/Unit/BitbucketTest.php b/packages/vcs/tests/Unit/BitbucketTest.php index d5d7fd977..7c5fe5ba8 100644 --- a/packages/vcs/tests/Unit/BitbucketTest.php +++ b/packages/vcs/tests/Unit/BitbucketTest.php @@ -26,6 +26,19 @@ final class BitbucketTest extends Base protected static string $pushEventName = 'repo:push'; protected static string $pullRequestEventName = 'pullrequest:created'; + /** + * Bitbucket names the action in the event rather than the payload, and + * has no reopen event. + * + * @var array + */ + protected static array $pullRequestActions = [ + 'pullrequest:created' => 'opened', + 'pullrequest:updated' => 'synchronize', + 'pullrequest:fulfilled' => 'closed', + 'pullrequest:rejected' => 'closed', + ]; + // Bitbucket's push payload carries no file lists at all protected static bool $reportsAffectedFilesInPushEvent = false; @@ -38,7 +51,13 @@ protected function signWebhookPayload(string $payload, string $secret): string return 'sha256=' . hash_hmac('sha256', $payload, $secret); } - protected function pushPayload(string $branch, array $added = [], array $removed = [], array $modified = [], bool $created = false, bool $deleted = false): string + #[\Override] + protected function pullRequestEventFor(string $action): string + { + return $action; + } + + protected function pushPayload(string $branch, array $added = [], array $removed = [], array $modified = [], bool $created = false, bool $deleted = false, array $olderCommits = []): string { $ref = [ 'type' => 'branch', @@ -51,6 +70,16 @@ protected function pushPayload(string $branch, array $added = [], array $removed ], ]; + // The adapter reads the head off new.target, so the commit list is + // there to prove the first commit listed is not taken for it + $commits = array_map(fn(string $hash): array => [ + 'hash' => $hash, + 'message' => 'Older commit', + 'author' => ['raw' => 'Older Author '], + 'links' => ['html' => ['href' => self::REPOSITORY_URL . '/commits/' . $hash]], + ], $olderCommits); + $commits[] = $ref['target']; + // A created branch has no old state and a deleted one no new state. The // file lists go unused, Bitbucket naming no files in a push. return (string) json_encode([ @@ -62,12 +91,16 @@ protected function pushPayload(string $branch, array $added = [], array $removed 'closed' => $deleted, 'old' => $created ? null : $ref, 'new' => $deleted ? null : $ref, + 'commits' => $commits, ]], ], ]); } - protected function pullRequestPayload(bool $external = false): string + /** + * The event names the action, so the payload is the same for every one. + */ + protected function pullRequestPayload(bool $external = false, string $action = 'pullrequest:created'): string { return (string) json_encode([ 'actor' => $this->eventActor(), @@ -156,22 +189,4 @@ public function testGetEventsReportsEveryPushedBranch(): void $this->assertSame([], $this->vcsAdapter->getEvents(self::$pushEventName, $tagsOnly)); } - - public function testGetEventPullRequestActionMapping(): void - { - $mapping = [ - 'pullrequest:created' => 'opened', - 'pullrequest:updated' => 'synchronize', - 'pullrequest:fulfilled' => 'closed', - 'pullrequest:rejected' => 'closed', - ]; - - foreach ($mapping as $event => $action) { - $events = $this->vcsAdapter->getEvents($event, $this->pullRequestPayload()); - $this->assertCount(1, $events); - $result = $events[0]; - - $this->assertSame($action, $result['action'], "event '{$event}' should map to '{$action}'"); - } - } } diff --git a/packages/vcs/tests/Unit/GitHubTest.php b/packages/vcs/tests/Unit/GitHubTest.php index bf05e963d..9b1e45fa1 100644 --- a/packages/vcs/tests/Unit/GitHubTest.php +++ b/packages/vcs/tests/Unit/GitHubTest.php @@ -25,8 +25,17 @@ protected function signWebhookPayload(string $payload, string $secret): string return 'sha256=' . hash_hmac('sha256', $payload, $secret); } - protected function pushPayload(string $branch, array $added = [], array $removed = [], array $modified = [], bool $created = false, bool $deleted = false): string + protected function pushPayload(string $branch, array $added = [], array $removed = [], array $modified = [], bool $created = false, bool $deleted = false, array $olderCommits = []): string { + $repositoryUrl = 'https://github.com/' . self::EVENT_OWNER . '/' . self::EVENT_REPOSITORY_NAME; + + $olderEntries = array_map(fn(string $hash): array => [ + 'id' => $hash, + 'message' => 'Older commit', + 'url' => $repositoryUrl . '/commit/' . $hash, + 'author' => ['name' => 'Older Author', 'email' => 'older@example.com'], + ], $olderCommits); + return (string) json_encode([ 'created' => $created, 'deleted' => $deleted, @@ -38,17 +47,17 @@ protected function pushPayload(string $branch, array $added = [], array $removed 'name' => self::EVENT_REPOSITORY_NAME, 'full_name' => self::EVENT_OWNER . '/' . self::EVENT_REPOSITORY_NAME, 'private' => true, - 'html_url' => 'https://github.com/' . self::EVENT_OWNER . '/' . self::EVENT_REPOSITORY_NAME, + 'html_url' => $repositoryUrl, 'owner' => ['name' => self::EVENT_OWNER, 'login' => self::EVENT_OWNER], ], 'installation' => ['id' => 1234], 'head_commit' => [ 'id' => self::EVENT_COMMIT_HASH, 'message' => self::EVENT_COMMIT_MESSAGE, - 'url' => 'https://github.com/' . self::EVENT_OWNER . '/' . self::EVENT_REPOSITORY_NAME . '/commit/' . self::EVENT_COMMIT_HASH, + 'url' => $repositoryUrl . '/commit/' . self::EVENT_COMMIT_HASH, 'author' => ['name' => self::EVENT_AUTHOR_NAME, 'email' => self::EVENT_AUTHOR_EMAIL], ], - 'commits' => [[ + 'commits' => [...$olderEntries, [ 'id' => self::EVENT_COMMIT_HASH, 'added' => $added, 'removed' => $removed, @@ -61,12 +70,12 @@ protected function pushPayload(string $branch, array $added = [], array $removed ]); } - protected function pullRequestPayload(bool $external = false): string + protected function pullRequestPayload(bool $external = false, string $action = 'opened'): string { $headOwner = $external ? 'someone-else' : self::EVENT_OWNER; return (string) json_encode([ - 'action' => 'opened', + 'action' => $action, 'number' => self::EVENT_PULL_REQUEST_NUMBER, 'pull_request' => [ 'id' => 1303283688, diff --git a/packages/vcs/tests/Unit/GitLabTest.php b/packages/vcs/tests/Unit/GitLabTest.php index c17825fa5..79c72c5a4 100644 --- a/packages/vcs/tests/Unit/GitLabTest.php +++ b/packages/vcs/tests/Unit/GitLabTest.php @@ -15,6 +15,19 @@ final class GitLabTest extends Base protected static string $pushEventName = 'Push Hook'; protected static string $pullRequestEventName = 'Merge Request Hook'; + /** + * GitLab names merge request actions as verbs, and a merged one is closed. + * + * @var array + */ + protected static array $pullRequestActions = [ + 'open' => 'opened', + 'reopen' => 'reopened', + 'update' => 'synchronize', + 'close' => 'closed', + 'merge' => 'closed', + ]; + protected function createAdapter(): GitLab { return new GitLab(new Cache(new None())); @@ -24,11 +37,18 @@ protected function signWebhookPayload(string $payload, string $secret): string return $secret; } - protected function pushPayload(string $branch, array $added = [], array $removed = [], array $modified = [], bool $created = false, bool $deleted = false): string + protected function pushPayload(string $branch, array $added = [], array $removed = [], array $modified = [], bool $created = false, bool $deleted = false, array $olderCommits = []): string { $blank = str_repeat('0', 40); $repositoryUrl = 'http://example.com/' . self::EVENT_OWNER . '/' . self::EVENT_REPOSITORY_NAME; + $olderEntries = array_map(fn(string $hash): array => [ + 'id' => $hash, + 'message' => 'Older commit', + 'url' => $repositoryUrl . '/-/commit/' . $hash, + 'author' => ['name' => 'Older Author', 'email' => 'older@example.com'], + ], $olderCommits); + return (string) json_encode([ 'object_kind' => 'push', 'ref' => 'refs/heads/' . $branch, @@ -43,7 +63,7 @@ protected function pushPayload(string $branch, array $added = [], array $removed 'namespace' => self::EVENT_OWNER, 'web_url' => $repositoryUrl, ], - 'commits' => $deleted ? [] : [[ + 'commits' => $deleted ? [] : [...$olderEntries, [ 'id' => self::EVENT_COMMIT_HASH, 'message' => self::EVENT_COMMIT_MESSAGE, 'url' => $repositoryUrl . '/-/commit/' . self::EVENT_COMMIT_HASH, @@ -55,7 +75,7 @@ protected function pushPayload(string $branch, array $added = [], array $removed ]); } - protected function pullRequestPayload(bool $external = false): string + protected function pullRequestPayload(bool $external = false, string $action = 'open'): string { return (string) json_encode([ 'object_kind' => 'merge_request', @@ -68,8 +88,7 @@ protected function pullRequestPayload(bool $external = false): string 'object_attributes' => [ 'iid' => self::EVENT_PULL_REQUEST_NUMBER, 'title' => 'Test MR', - // GitLab calls it 'open' and normalizes to 'opened' - 'action' => 'open', + 'action' => $action, 'source_branch' => self::EVENT_HEAD_BRANCH, 'target_branch' => self::$defaultBranch, 'source_project_id' => $external ? 456 : (int) self::EVENT_REPOSITORY_ID, @@ -84,65 +103,4 @@ protected function pullRequestPayload(bool $external = false): string ], ]); } - - public function testGetEventPushMatchesCheckoutSha(): void - { - $payload = json_encode([ - 'object_kind' => 'push', - 'ref' => 'refs/heads/main', - 'checkout_sha' => 'def456', - 'project' => [ - 'name' => 'test-repo', - 'namespace' => 'test-org', - ], - 'commits' => [ - [ - 'id' => 'abc123', - 'message' => 'Older commit', - 'url' => 'http://example.com/commit/abc123', - 'author' => ['name' => 'Old Author'], - ], - [ - 'id' => 'def456', - 'message' => 'Head commit', - 'url' => 'http://example.com/commit/def456', - 'author' => ['name' => 'Head Author'], - ], - ], - ]); - - if ($payload === false) { - $this->fail('Failed to encode JSON payload'); - } - - $events = $this->vcsAdapter->getEvents('Push Hook', $payload); - $this->assertCount(1, $events); - $result = $events[0]; - - $this->assertIsArray($result); - $this->assertSame('def456', $result['commitHash']); - $this->assertSame('Head Author', $result['headCommitAuthorName']); - $this->assertSame('Head commit', $result['headCommitMessage']); - $this->assertSame('http://example.com/commit/def456', $result['headCommitUrl']); - } - - public function testGetEventPullRequestActionMapping(): void - { - foreach (['open' => 'opened', 'reopen' => 'reopened', 'update' => 'synchronize', 'close' => 'closed', 'merge' => 'closed'] as $native => $mapped) { - $payload = json_encode([ - 'object_kind' => 'merge_request', - 'project' => ['id' => 1, 'name' => 'r', 'namespace' => 'o', 'web_url' => 'http://example.com/o/r'], - 'object_attributes' => ['iid' => 1, 'action' => $native, 'source_branch' => 'f', 'target_branch' => 'main'], - ]); - - if ($payload === false) { - $this->fail('Failed to encode JSON payload'); - } - - $events = $this->vcsAdapter->getEvents('Merge Request Hook', $payload); - $this->assertCount(1, $events); - $result = $events[0]; - $this->assertSame($mapped, $result['action'], "native action '{$native}' should map to '{$mapped}'"); - } - } } diff --git a/packages/vcs/tests/Unit/GiteaBase.php b/packages/vcs/tests/Unit/GiteaBase.php index 9e193453c..6e407bad5 100644 --- a/packages/vcs/tests/Unit/GiteaBase.php +++ b/packages/vcs/tests/Unit/GiteaBase.php @@ -10,15 +10,34 @@ */ abstract class GiteaBase extends Base { + /** + * Gitea says 'synchronized' for a pushed head, which the adapter normalizes. + * + * @var array + */ + protected static array $pullRequestActions = [ + 'opened' => 'opened', + 'reopened' => 'reopened', + 'synchronized' => 'synchronize', + 'closed' => 'closed', + ]; + protected function signWebhookPayload(string $payload, string $secret): string { return hash_hmac('sha256', $payload, $secret); } - protected function pushPayload(string $branch, array $added = [], array $removed = [], array $modified = [], bool $created = false, bool $deleted = false): string + protected function pushPayload(string $branch, array $added = [], array $removed = [], array $modified = [], bool $created = false, bool $deleted = false, array $olderCommits = []): string { $repositoryUrl = 'http://gitea:3000/' . self::EVENT_OWNER . '/' . self::EVENT_REPOSITORY_NAME; + $olderEntries = array_map(fn(string $hash): array => [ + 'id' => $hash, + 'message' => 'Older commit', + 'url' => $repositoryUrl . '/commit/' . $hash, + 'author' => ['name' => 'Older Author', 'email' => 'older@example.com'], + ], $olderCommits); + return (string) json_encode([ 'ref' => 'refs/heads/' . $branch, 'before' => 'abc123', @@ -43,7 +62,7 @@ protected function pushPayload(string $branch, array $added = [], array $removed 'url' => $repositoryUrl . '/commit/' . self::EVENT_COMMIT_HASH, 'author' => ['name' => self::EVENT_AUTHOR_NAME, 'email' => self::EVENT_AUTHOR_EMAIL], ], - 'commits' => [[ + 'commits' => [...$olderEntries, [ 'id' => self::EVENT_COMMIT_HASH, 'added' => $added, 'removed' => $removed, @@ -52,7 +71,7 @@ protected function pushPayload(string $branch, array $added = [], array $removed ]); } - protected function pullRequestPayload(bool $external = false): string + protected function pullRequestPayload(bool $external = false, string $action = 'opened'): string { $repositoryUrl = 'http://gitea:3000/' . self::EVENT_OWNER . '/' . self::EVENT_REPOSITORY_NAME; $headRepository = $external @@ -60,7 +79,7 @@ protected function pullRequestPayload(bool $external = false): string : self::EVENT_OWNER . '/' . self::EVENT_REPOSITORY_NAME; return (string) json_encode([ - 'action' => 'opened', + 'action' => $action, 'number' => self::EVENT_PULL_REQUEST_NUMBER, 'pull_request' => [ 'id' => 1,