Feat/normalize adapter behavior#108
Conversation
…atus error handling
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fb650bac0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| $this->markTestSkipped('GitHub adapter does not support getUser by username'); | ||
| } | ||
|
|
||
| public function testGetOwnerName(): void |
There was a problem hiding this comment.
This adds a second testGetOwnerName() method to GitHubTest while the same method already exists earlier in the class, so PHP cannot even load the test file (php -l tests/VCS/Adapter/GitHubTest.php reports Cannot redeclare Utopia\Tests\Adapter\GitHubTest::testGetOwnerName()). Any PHPUnit run that includes this file will fail before executing tests until one of the duplicate methods is removed or renamed.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR normalizes the return shapes across all three VCS adapters so that shared base tests can cover previously adapter-specific cases, and fixes
Confidence Score: 4/5Safe to merge after confirming the Gitea getCommitStatuses field rename is intentional and any upstream consumers are updated. The GitLab and GitHub normalization changes are clean and well-tested. The Gitea getCommitStatuses change silently renames the status field to state in the returned array, which would break any caller checking $status['status']. The roundtrip test that caught this was removed and no base equivalent was added to verify the new field name, making the regression easy to miss in production. src/VCS/Adapter/Git/Gitea.php — the getCommitStatuses normalization changes the output schema without a corresponding test that asserts the new field name. Important Files Changed
Reviews (7): Last reviewed commit: "fix: increase assertEventually timeout f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Seeing this diff is slightly worrying:
We removed 2k, adding 1k to base. While techincally it could be OK, its a bit scarrry.
There was a problem hiding this comment.
I ran agent to do a diff review, here is his comment
Review Findings: Coverage Lost During Test Migration
I compared the upstream main branch against the PR branch (feat/normalize-adapter-behavior), specifically looking for tests removed from adapter-specific files that were not restored in tests/VCS/Base.php. Here are the concrete mistakes I found:
- testCreatePrivateRepository — privacy assertion dropped
Impact: All adapters (GitHub, GitLab, Gitea)
All three adapter tests previously verified that a private repository was actually created as private:
- GitHub: $this->assertTrue($result['private'])
- GitLab: $this->assertSame('private', $result['visibility']) (+ verified via getRepository)
- Gitea: $this->assertTrue($result['private']) (+ verified via getRepository)
The migrated test in Base.php only asserts that the result has a name key. It never checks the privacy flag, so a broken adapter that ignores the $private parameter would still pass.
- testUpdateCommitStatus — status verification dropped
Impact: GitLab and Gitea (GitHub already had the weak version)
Upstream GitLab and Gitea tests called getCommitStatuses() after updateCommitStatus() to confirm the status was recorded:
- GitLab: asserted state array contained 'success'
- Gitea: looped through statuses, found the 'ci/tests' context, and asserted status === 'success' and description === 'Tests passed'
The migrated test in Base.php only asserts $this->assertTrue(true) after the update call. It does not verify the commit status was persisted at all.
- testCreateRepositoryWithInvalidName — removed from Gitea/Forgejo, not added to Base
Impact: Gitea and Forgejo
Gitea upstream had a negative test for invalid repository names ('invalid name with spaces'). It was deleted in the PR and never moved to Base.php.
GitLabTest still keeps its own copy, but GiteaTest (and ForgejoTest, which extends it) no longer runs it. - testCreateRepository — public visibility assertion dropped
Impact: GitHub, GitLab, Gitea
All adapter-specific tests previously asserted the created public repo was actually public:
- GitHub: $this->assertFalse($result['private'])
- GitLab: $this->assertFalse($result['visibility'] === 'private')
- Gitea: $this->assertFalse($result['private'])
The Base version only checks name and pushed_at, so this coverage is also lost.
- testGetOwnerName — null $repositoryId case lost for GitLab
Impact: GitLab
GitLab upstream had two tests: - testGetOwnerName() — called getOwnerName('', null) (no repo ID)
- testGetOwnerNameWithRepositoryId() — called getOwnerName('', $repositoryId)
Base only has one test that passes a $repositoryId. The null / no-arg path for GitLab is no longer exercised.
Summary Table
Test / Coverage
Assert repo is private (testCreatePrivateRepository)
Assert repo is public (testCreateRepository)
Verify commit status after update (testUpdateCommitStatus)
Invalid repository name (testCreateRepositoryWithInvalidName)
getOwnerName('', null)
There was a problem hiding this comment.
Please also try from your side, with agent (after fixes), see if on another try he can spot anything else.
Also keep in mind its AI, he can be wrong. If something feels irrelevant to you, feel free to ignore.
… status verification, and GitLab getOwnerName fallback
| public function testGetUser(): void | ||
| { | ||
| $result = $this->vcsAdapter->getUser('root'); | ||
| $this->assertIsArray($result); | ||
| $this->assertArrayHasKey('id', $result); | ||
| $this->assertArrayHasKey('username', $result); | ||
| } |
There was a problem hiding this comment.
username key assertion will fail for Gogs (and older Gitea/Forgejo)
The base test now asserts assertArrayHasKey('username', $result), but Gitea::getUser() returns the raw API response whose canonical field is login, not username. The old adapter-specific test for Gitea explicitly checked assertArrayHasKey('login', $ownerInfo). Gogs (whose API is a separate, older codebase) almost certainly only returns login. GogsTest extends GiteaTest and inherits this test without overriding it, so testGetUser will fail on Gogs. Newer Gitea/Forgejo versions do expose username as an alias, but the divergence from the removed test is worth locking down — either normalise the response in Gitea::getUser() to always include a username key, or assert login (and skip testGetUser for GitLab which is the only adapter that uses username).
Meldiron
added
test
…API eventual consistency
Meldiron
added
test
…d fix flaky branch tests
| throw new Exception("Failed to get commit statuses: HTTP {$responseHeadersStatusCode}", $responseHeadersStatusCode); | ||
| } | ||
|
|
||
| return $response['body'] ?? []; | ||
| $responseBody = $response['body'] ?? []; | ||
| if (!is_array($responseBody)) { | ||
| return []; | ||
| } | ||
|
|
||
| $statuses = []; | ||
| foreach ($responseBody as $status) { | ||
| $statuses[] = [ | ||
| 'state' => $status['status'] ?? '', | ||
| 'description' => $status['description'] ?? '', | ||
| 'target_url' => $status['target_url'] ?? '', | ||
| 'context' => $status['context'] ?? '', | ||
| ]; | ||
| } | ||
|
|
||
| return $statuses; | ||
| } | ||
| } |
There was a problem hiding this comment.
getCommitStatuses field rename breaks existing callers
The normalization renames the commit-state field from status (the raw Gitea/Gogs API field) to state. Any caller that was reading $result['status'] from Gitea::getCommitStatuses() — including the now-deleted testUpdateCommitStatus in GiteaTest, which asserted $status['status'] ?? '' — will silently receive null/empty string instead. The PR description only lists GitLab and GitHub adapter fixes; this Gitea change is undocumented and the roundtrip test that would have caught it was removed without a base-level replacement that verifies the state field.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Meldiron
added
test
2 participants
Normalizes adapter behavior to enable shared Base tests:
Adapter fixes:
GitLab::searchRepositories — was returning flat array, now returns {items, total} consistent with Gitea/GitHub
GitLab::searchRepositories — was returning [] for invalid owner, now returns {items: [], total: 0}
GitHub::searchRepositories — was throwing for invalid owner, now returns empty results
GitHub::updateCommitStatus — was silently ignoring API errors, now throws on failure
Tests moved to Base:
testUpdateCommitStatusWithInvalidCommit
testUpdateCommitStatusWithNonExistingRepository
testSearchRepositoriesNoResults
testSearchRepositoriesInvalidOwner
Depends on: feat/unified-base-tests