test: share every non provider specific test through Base, and declare what differs - #126
Merged
Conversation
GitHub and Gitea both throw on a payload that isn't valid JSON, while GitLab returned an empty array - indistinguishable from an event it simply doesn't handle. That inconsistency was the only reason the invalid-payload test had to live in the Gitea class rather than the shared base. Unsupported event names still return an empty array, as before.
Base tests papered over provider differences with fallbacks, so they passed without pinning anything down: owner was read as owner.login or namespace.path or bust, visibility as a bool or a string, a pull request number as iid or number or 0, a user handle as username or login or empty, pushed_at as null or a timestamp, and a pull request state as open or opened. Each of those is now a declared per-provider fact - an overridable helper or a static - so every adapter asserts exactly what its provider reports and a wrong shape fails instead of falling through. The same declarations let these move into Base, which is where the duplication actually was: - webhook header names, which also replaces two Base tests that only checked the names were non-empty - getOwnerName with a missing, zero and null repository id - createRepository with an invalid name, and clone commands for a tag - searchRepositories matching by name, now also covering GitHub - getEvent on a malformed payload, now also covering GitLab Tests defined in more than one adapter class: 13 -> 8. The rest are genuinely provider specific - webhook payloads, signature schemes, presigned URL shapes.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis follow-up centralizes provider-independent adapter tests and makes provider differences explicit.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (15): Last reviewed commit: "Default every capability to supported in..." | Re-trigger Greptile |
…ehave Three assertions were written from assumption rather than observation, and CI corrected two of them: - pushed_at was tolerated as null or a timestamp, and I had pinned the null case to GitHub. GitHub actually reports a timestamp for a repository with no commits too, so the null branch was never reachable for any provider - every adapter now has to report a parseable timestamp. - a failed clone was accepted if the command exited non-zero or checked out nothing. On Gitea and Gogs it exits zero, because the command sets up a local repository before discovering the remote is missing, so the checkout being empty is the only real signal and is now the assertion. - supported webhook scopes were only asserted to be non-empty; the exact set is now declared per provider, with the installation scope only on GitHub. Also stops the two search tests orphaning their first repository when the second one fails to create, and asserts a commit url points at the repository it came from.
The cleanup helper caught every throwable, so an auth, transport or provider failure during teardown would pass silently and leave the repository behind for later runs to trip over. Adapters carry the HTTP status as the exception code, so a repository that was never created is a 404 and stays tolerated; everything else surfaces.
Rethrowing on the first deletion left the second repository untouched, so surfacing one failure created another leak. Cleanup now takes every repository, skips the ones that were never created, and reports whatever actually failed once all of them have been attempted.
GitLab answered a commit status POST with 403 in CI: the test read the latest commit directly and wrote to it straight away, while the shared commit status test that waits through getLatestCommitEventually passed in the same run. The two GitLab commit status tests and Gitea's tag test now use that helper too, which is what every other test in the suite does.
Reporting a cleanup failure from finally overwrote whatever the test body had already raised. Cleanup is now two things: deleteRepositories, asserted on the path where the test passed, and discardRepositories, silent on the path where a failure is already on its way out.
Three adapters implement getRepositoryPresignedUrl() but nothing declared it, so the shared tests could not call it. A default on Git makes the contract reachable while leaving adapters that do not offer archives - Gogs, and anything being written against this class - working as they are.
Following review: the adapter classes were still carrying tests that only looked provider specific. Moved to Base, with the parts that genuinely differ declared rather than duplicated: - both webhook round trips, through an awaitWebhook() helper. This also puts them near the end of a run again, which they lost when earlier tests moved to Base and inherited tests started running after a subclass's own - validateWebhookEvent, via a signWebhookPayload() each adapter implements: GitHub prefixes its HMAC, Gitea sends a plain one, GitLab sends the secret verbatim - presigned urls, commit statuses, createTag, tree with a slash in the branch name, search pagination, listTags on a commit-less repository, hasAccessToAllRepositories, getInstallationRepository, getOwnerName with an id that does not exist Two provider facts came out of running it rather than reading the code: Gitea reports a newly opened pull request as 'synchronized' and GitLab as 'synchronize', because both follow the opened event with a sync for the head they just pushed and the catcher keeps only the last delivery. No test is defined in two adapter classes any more. GiteaTest 634 -> 300 lines, GitLabTest 682 -> 461. What stays adapter specific is genuinely provider bound: hand-built getEvent payloads, GitHub's check runs, pagination, blob SHA and case sensitivity, GitLab's namespaces and path sentinels, Gitea's avatar host and delete-then-read.
CI answered both: GitHub redirects archive downloads to codeload, whose paths read legacy.tar.gz and legacy.zip rather than tarball and zipball, so the shared defaults already described it and the override was wrong. And getOwnerName ignores the repository id entirely on GitHub, resolving the owner from the installation, so an id that does not exist has no meaning there - skipped like the other getOwnerName cases.
GitHubTest carried 22 skip methods and GogsTest 16, all saying the same thing in four lines each: this provider does not have that API. They are now capabilities on Base - pull request creation and lookup, commit statuses and reading them back, tags, user lookup, repository languages, webhook delivery, resolving an owner from a repository id, rejecting invalid repository names - and the shared tests skip themselves when a capability is missing. GitHubTest 738 -> 661 lines, GogsTest 141 -> 82. Cleanup also became one policy. A transient GitLab 500 while deleting a repository failed testListRepositoryContents in CI even though the test itself had passed, and 49 other finally blocks could do the same. Cleanup is best effort everywhere now; deleting is asserted by the delete tests.
Check runs were 276 of GitHubTest's lines. They are a GitHub-only API, so Git declares them the way it declares presigned urls - a default that reports them unsupported - and the tests live in Base behind a supportsCheckRuns capability. Any adapter that gains check runs inherits the tests instead of copying them. GitHub's last two overrides went the same way: - getLatestCommit only differed by asserting the author avatar and profile url, now two capabilities, since GitLab reports neither and Gitea reports an avatar but no url - updateCommitStatus only differed by not reading the status back, so writing is gated on supportsCommitStatuses and reading on supportsCommitStatusLookup. GitHub gains the write test it used to override away GitHubTest 738 -> 328 lines. What is left cannot be shared: hand-built webhook payloads, GitHub's own listBranches signature, the git blob SHA, case sensitive paths, installation-scoped getOwnerName, and language stats that need the inconclusive handling.
Moving GitLab's path sentinel tests into Base showed the contract only held for GitLab: '.' and './' returned the root listing there and nothing on Gitea, because #121 fixed the path handling in one adapter. The same call answered differently depending on the provider. normalizeRepositoryPath() moves off GitLab onto Git, and Gitea and GitHub normalize before building their contents urls, so a caller passing '.', './' or 'src//' gets the same answer everywhere. Gitea went from failing all three sentinel tests to passing them. listNamespaces moves to Base the same way check runs did - a default on Git reporting it unsupported, plus a capability - so GitLabTest is 682 -> 367 lines.
Sharing testGetPullRequestWithInvalidNumber ran it against GitHub for the first time, where it had been skipped, and GitHub returned the 404 body as if it were a pull request while Gitea and GitLab both throw. Same shape as createRepository not checking its status code. Also waits for both branches to be listable before paging through them, so GitHub not having indexed the second one yet stops failing the pagination test.
None of them were really GitHub specific: - getOwnerName differed only in which argument the provider reads, so the shared test passes both an installation id and a repository id and each adapter uses the one it resolves owners from - language stats differ in when they appear, not what they are, so the wait and the inconclusive skip are behind computesLanguagesAsynchronously - case sensitive paths and the git blob SHA are git behaviours: Gitea and GitLab both hold to them, confirmed by running it GitHubTest 738 -> 263 lines. What is left is the hand-built webhook payloads, its own listBranches signature, and the provider facts.
Every adapter's getEvent already reports the same normalized keys - branch, branchCreated, branchDeleted, repositoryId, repositoryName, owner, commitHash, headCommit*, affectedFiles, external - so the only thing that was provider specific about these tests was the payload going in. Base now owns the assertions and declares two builders each adapter fills in from the same EVENT_* facts: a push payload and a pull request payload. GitHub signals a created branch with a flag, GitLab with an all-zero sha, and each shapes the owner and the pull request number its own way, which is exactly what the builders express. That covers push, pull request, branch created, branch deleted and external pull requests for every provider from one place. GitLab keeps the two that are its own semantics - matching checkout_sha and its action mapping - and GitHub keeps installation events. GitHubTest 738 -> 244 lines, GitLabTest 682 -> 265, GiteaTest 634 -> 146, GogsTest 131 -> 74.
Review flagged that cleanup swallows every deletion failure, which is fair: a leaked repository was invisible. Throwing instead is what the previous round did, and a transient GitLab 500 during teardown then failed a test that had actually passed. Cleanup now stays out of the pass/fail decision and writes to the log when it could not delete something, so a leak is visible without a provider hiccup deciding whether a test failed. A repository that was never created is still nothing to report.
Review wants a cleanup failure to affect the result, and it is right that a repository left behind contaminates later runs. Making it throw is what an earlier round did, and a transient GitLab 500 during teardown then failed a test that had passed - which is why it became best effort. Retrying resolves both: a hiccup is retried away, and a deletion that still will not go through after three attempts fails the test with the names of the repositories it could not remove. A repository that was never created stays silent.
HarshMN2345
changed the title
Meldiron
reviewed
Jul 30, 2026
|
|
||
| protected static bool $supportsCheckRuns = false; | ||
|
|
||
| protected static bool $supportsNamespaceListing = false; |
Contributor
There was a problem hiding this comment.
Lets mark all flags enabled to maximum in base file. It's adapter's responsiblity to mark some support as limited or not supported.
That will help us find and address those in future more easily
Meldiron
approved these changes
Jul 30, 2026
Removing tests by hand left GogsTest with headings introducing skip methods that are capabilities now, and GitHubTest with 44 blank lines at the end. Also collapses the other gaps the deletions opened up.
Per review: Base should describe the whole contract, and an adapter should be the thing that says where its provider falls short. Check runs, namespace listing, installation repository lookup and commit author links defaulted to unsupported, so a provider that cannot do them said nothing and the gap was invisible. They now default to supported, and each adapter states its own gaps - GitLab has no check runs, installation lookup or author links, Gitea has no check runs, namespaces or installation lookup and reports no author url. Anything a provider does not support is a line in its own class from now on.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #124 on two pieces of review feedback: the shared tests leaned on fallbacks that let them pass without pinning provider behaviour down, and the adapter classes were still large.
Every test that is not genuinely provider specific now lives in
Base, and what differs per provider is declared rather than duplicated.tests/VCS/Adapter/GitHubTest.phptests/VCS/Adapter/GitLabTest.phptests/VCS/Adapter/GiteaTest.phptests/VCS/Adapter/GogsTest.phptests/VCS/Base.phpNo test is defined in more than one adapter class any more, and every adapter runs more tests than before: gitlab 84 → 102, gitea 80 → 102, gogs → 102, github 69 → 73 active.
Fallbacks and looseness
Basewas guessing at provider shapes at runtime, so a wrong shape fell through to the next branch instead of failing:owner.login, elsenamespace.path, else failownerOf(), overridden by GitLab, which reports a namespaceprivatebool, else avisibilitystringisPrivate(), overridden by GitLab, which reports a stringiid ?? number ?? 0pullRequestNumberOf(), overridden by GitLabusername ?? login ?? ''$userHandleFieldpushed_ataccepted as null or a timestampopenoropened$openPullRequestStateWhat moved into Base
Repositories, files, trees, contents, branches, tags, commits, commit statuses, comments, pull requests, clone commands, searching,
getUser,getOwnerName, presigned URLs, check runs,listNamespaces, both live webhook round trips, and the webhook payload parsing.Three mechanisms made that possible:
markTestSkippedmethods and GogsTest 16, all saying "this provider has no such API" in four lines each. They are now declarations -$supportsPullRequestCreation,$supportsCommitStatusLookup,$supportsTags,$supportsCheckRuns,$supportsWebhookDelivery,$createsEmptyRepositoriesand so on - and the shared tests skip themselves.signWebhookPayload()(GitHub prefixes its HMAC, Gitea sends a plain one, GitLab sends the secret verbatim),ownerOf(),isPrivate(),pullRequestNumberOf().getEvent()already return the same normalized keys, so the only provider-specific part of those tests was the payload going in. Each adapter builds a push and a pull request payload from sharedEVENT_*facts;Baseowns the assertions. GitHub signals a created branch with a flag and GitLab with an all-zero sha, which is exactly what the builders express.Adapter changes
Sharing a test repeatedly meant running it against a provider for the first time, which surfaced four inconsistencies:
createRepositorynever checked its status code on Gitea, Gogs and GitHub, so an error response was returned as if it were a created repository. GitLab already threw. The old Gitea test hid this: it wrapped$this->fail()incatch (\Exception), which swallowed the failure it was meant to report.GitLab::getEvent()returned an empty array for a payload that is not valid JSON while GitHub and Gitea throw, making a malformed payload indistinguishable from an unhandled event. Unsupported event names still return an empty array.'.','./'and'src//'resolved differently per provider. Fix GitLab adapter treating './' as a literal path instead of repository root #121 fixed the path handling in GitLab only, so the same call returned the root listing there and nothing on Gitea.normalizeRepositoryPath()moves ontoGit, and Gitea and GitHub normalize too.GitHub::getPullRequestreturned the 404 body for a pull request that does not exist, where Gitea and GitLab throw.Gitalso gained defaults that report a capability as unsupported forgetRepositoryPresignedUrl(), the three check run methods andlistNamespaces(). They are deliberately not abstract, so adapters that do not offer them - including any being written against this class - keep working.Verification
All five adapter suites, the linter and CodeQL pass.
composer check(PHPStan level 8) is clean.Worth a reviewer's attention:
createRepository,getPullRequestandGitLab::getEventnow throw where they previously returned a value. That is the intended fix, but it is a behaviour change for callers.getEvent()['action']reportssynchronizedon Gitea andsynchronizeon GitHub and GitLab. Left alone here since it changes behaviour for consumers, but a consumer switching onsynchronizesilently misses Gitea.