test: unify adapter tests into a shared Base and normalize adapter behavior - #124
Merged
Conversation
Contributor
Greptile SummaryThe PR consolidates adapter integration tests and normalizes several provider behaviors.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (19): Last reviewed commit: "Wait for GitHub to index a commit before..." | Re-trigger Greptile |
…atus error handling
… status verification, and GitLab getOwnerName fallback
…API eventual consistency
…d fix flaky branch tests
GitHub::searchRepositories() inferred failure from a missing response key, so a rate limit or auth error looked the same as "no results". Check the status code instead: throw with it on failures, otherwise return the empty items/total shape the other adapters use. Also aligns GitLab::getOwnerName() with Gitea: a non-positive repository id means "no repository", not a lookup of project 0.
The branch was cut before listTags, presigned URLs and the GitLab event parity work landed, so replaying it dropped those tests. Restores them and folds the still-duplicated cases into Base: - listTags, the empty-repository branch listing and getUser are now shared - adapter classes name the account to look up via $existingUser instead of renaming the Gitea/Forgejo/Gogs admin to root in docker-compose - webhook tests read the header names off the adapter instead of repeating them per test class - getEventPush, getEventPullRequest and validateWebhookEvent are abstract in Base, since payloads and signatures are provider specific
HarshMN2345
force-pushed
the
test/unify-adapter-base-tests
branch
from
July 27, 2026 13:03
5943f49 to
207c7cd
Compare
An audit of the diff against main turned up assertions that were dropped rather than moved: - commit status now has to come back under the context it was written with, with the description intact, which is what covers the Gitea normalization this branch adds - delete failures have to carry the HTTP status as the exception code - getRepository has to throw RepositoryNotFound, not any Exception - createRepository's own response has to report the requested visibility and a parseable pushed_at, not just contain the key - getCommit has to report a commit author - a commit clone command has to name the commit it was asked for - GitHub keeps its git blob SHA check, and Gitea its search-matches-name and delete-then-read cases, which no shared test covers Also drops four GitHub overrides that were byte-identical to Base (one of them leaked its clone directory), a GitLab getEvent test that duplicated Base's, and uses the eventual-consistency helper in GitHub's commit status test.
- repositories are asserted to belong to the owner they were created under, via an ownerPath()/ownerOf() pair that handles GitLab's "id:path" owners and GitLab's namespace shape - getOwnerName is asserted exactly instead of by substring, which lets the Gitea-specific copy of the test go away - searchRepositories items are asserted to carry id, name, private and a usable pushed_at for every adapter, replacing GitLab's private copy - clone commands are asserted to set up sparse checkout, and commit clones to be shallow - pull request state has to read 'open' or 'opened', not merely be non-empty - the recursive tree has to be at least as large as the files put in it - webhook header names are pinned per provider, which is what covers the Forgejo and Gogs overrides - Gitea keeps its avatar-host check, GitLab a commit-less listTags case - Gogs::getPullRequestFiles now reports the pull request API as unsupported like its siblings instead of inheriting Gitea's endpoint Drops GiteaTest's comment workflow, which repeated the three shared comment tests over four repository lifecycles.
Only GitLab checked the status code, so Gitea, Gogs and GitHub returned whatever the API replied with — an error body looked like a created repository. The Gitea suite's testCreateRepositoryWithInvalidName hid this: it wrapped $this->fail() in catch (\Exception), which swallowed the failure it was supposed to report, so the test passed either way. All four now throw with the HTTP status as the exception code, and the Gitea-family test asserts that instead of being skipped.
Meldiron
reviewed
Jul 28, 2026
| docker compose --profile ${{ matrix.adapter }} up -d | ||
| sleep 15 | ||
|
|
||
| - name: Wait for the provider to settle |
Contributor
There was a problem hiding this comment.
Lets check with Cloud team if this can be done nicer - if they have some examples of similar logic elsewhere
Waiting for container to be healthly, different container for each matrix
Meldiron
approved these changes
Jul 28, 2026
The previous fix polled docker inspect from the workflow to work around GitLab flipping to "healthy" on a single successful check and then still returning 502s for a while, since a container's healthcheck only needs one success to transition out of "starting" - retries only govern how many consecutive failures are needed to go back to unhealthy afterwards. Move that requirement into the healthcheck itself instead: GitLab's test now demands 3 consecutive successful curls before Docker reports healthy, so the healthcheck can be trusted. Both workflows drop the custom polling step for docker compose's own `up -d --wait`, which blocks until every started service is healthy.
This reverts commit 0cb040d.
The last CI failure had GitLab go fully unreachable partway through a run that had already been passing tests, not just slow to start - a different failure mode than the boot-time flakiness the wait step handles, and one we have no data on. Log gitlab-ctl status after the test step, whether or not it passed, so the next occurrence shows which internal process (gitaly, puma, postgresql, etc.) was down at that point instead of just an HTTP error on our end. Diagnostic only: does not change the existing wait-for-healthy step.
gitlab-bootstrap already depends on gitlab: condition: service_healthy, and tests depends on gitlab-bootstrap completing - so Compose was already refusing to start the tests container before GitLab reported healthy. The only weak link was the healthcheck itself: one successful curl only proves nginx answered, not that the API behind it can serve requests, which is what let 502s through despite the dependency being wired correctly. Requiring 3 consecutive successful checks closes that gap without any custom polling. Both workflows now just call `up -d --wait`, which is redundant with the existing dependency chain but fails loudly with a clear timeout instead of a silent hang if something is ever wrong. Verified locally against a freshly wiped gitlab-data volume: healthy at 2m16s, 84/84 tests immediately after.
Same run that had a clean, error-free boot and dependency chain (no 502s, no connection failures) still saw both webhook tests time out waiting for delivery at 30s. GitLab queues hook deliveries through Sidekiq, which can still be warming up in the moments right after the instance becomes reachable - a different bottleneck than the one the healthcheck fix addresses. Widening to 60s.
Widening the timeout only cut the earlier failures in half (2 -> 1), and by the time it still failed GitLab had been up for minutes and processed plenty of other jobs already - a cold Sidekiq worker doesn't explain that. No service was down per the existing diagnostic either. Log the actual WebHookWorker entries from sidekiq's log and the catcher's own log when a gitlab test fails, so the next occurrence shows whether GitLab even attempted delivery, and whether it reached the catcher, instead of leaving both as open questions.
Two separate steps each re-checking matrix.adapter == 'gitlab' was redundant. One step now runs always(), and branches on the Run Tests step's own outcome for the failure-only part instead of a second if:.
Comparing matrix.adapter to a literal string in the workflow breaks silently if the adapter is ever renamed. Check whether a running gitlab container actually exists instead, verified against both a real gitlab run and a profile that doesn't start one.
It never actually explained a failure - it only confirmed nothing was down, which we already suspected - and the flake it was added for hasn't recurred since the timeout was widened. Speculative logging for a problem that isn't currently happening is better added later, informed by whatever that failure actually looks like, than guessed at now.
testListBranchesPagination created branches immediately after creating the first file, without waiting for GitHub to index that commit first. GitHub::createBranch resolves the source branch through getLatestCommit(), which throws a 422 when the commit isn't queryable yet - exactly the eventual-consistency gap the rest of the suite already guards against with getLatestCommitEventually(), just missing from this one test.
HarshMN2345
force-pushed
the
test/unify-adapter-base-tests
branch
from
July 28, 2026 09:56
d379d43 to
ce4df64
Compare
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
3 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.
Adapter changes
GitHub::updateCommitStatus()silently ignored API errors; it now throws with the HTTP status as the exception code, like every other adapter.GitHub::searchRepositories()inferred failure from a missing response key, so a rate limit or auth error was indistinguishable from "no results". It now checks the status code: a 422 (the search API rejecting an owner that does not exist) returns the emptyitems/totalshape, anything else >= 400 throws with the status.Gitea::getCommitStatuses()returned raw Gitea objects (status); it now returns the samestate/description/target_url/contextshape GitLab already returned. The contract is documented onGit::getCommitStatuses().GitLab::getOwnerName()treated a0repository id as a real id and looked up project 0. A non-positive id now means "no repository" and falls back to the authenticated user, matching Gitea.Tests
Basenow owns the shared cases: repositories, files, trees, contents, branches, tags, commits, commit status, comments, pull requests, clone commands,getUser,getOwnerName,searchRepositories.getEventPush,getEventPullRequestandvalidateWebhookEventare abstract inBase, following the existing convention for provider-specific tests, instead of skipped placeholders.getEventHeaderName()/getSignatureHeaderName()) instead of repeating them as test-class properties, so the advertised header is now covered too.$existingUser(utopiafor Gitea/Forgejo/Gogs,rootfor GitLab). Feat/normalize adapter behavior #108 renamed the Gitea/Forgejo/Gogs admin torootindocker-compose.ymlto make a sharedgetUsertest pass — that change is dropped.Net: 2823 lines deleted, 1442 added, and every adapter runs more active tests than before (GitHub 55 -> 68, GitLab 70 -> 84, Gitea/Forgejo 76 -> 78, Gogs 61 -> 62).
Two deltas worth calling out:
testCreateRepositoryWithInvalidNameis now skipped for Gitea/Forgejo/Gogs. The old version wrapped$this->fail()incatch (\Exception), which swallowed the failure — it passed regardless of behaviour, and Gitea does accept names with spaces. GitLab keeps a realexpectExceptionversion.GitHubTestskipsgetUserwith a precise reason:GitHub::getUser()returns the raw response envelope rather than the shared user shape, and does not throw for a missing user. Left alone here since callers may depend on it.GitHubTest::testListRepositoryLanguageswaits 60s and then reports skipped. GitHub computes language stats out of band with no guaranteed turnaround and reports none until it finishes, so a repository without stats says nothing about the adapter. Base stays strict for the self-hosted providers, where the value is synchronous.CI
The
gitlabjob was failing on transport errors (502s, connection resets) with every suite run: GitLab reports healthy and then flaps while it finishes starting, so thesleep 15in the workflow let tests start against an API that was not serving yet. Measured locally —/-/health,/-/readinessand/api/v4all flip to 200 at the same moment, so a deeper probe buys nothing; the problem is the flapping after the first healthy report.githubprofile, which has no containerGitLabTestretries the one-time group creation while the API is still coming upAdapter-level retries on 5xx would be the durable fix, but #103 is already adding those, so nothing here touches the adapters' error handling.
Verification
gitea,forgejo,gogsandgitlabsuites were run locally against the docker stack — all green (79/79, 79/79, 79/79, 84/84).composer lintandcomposer check(PHPStan level 8) pass. All five adapter jobs, the linter and CodeQL are green in CI.