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
Confidence Score: 5/5The changes are safe to merge; all three adapter fixes correctly align error and empty-result handling, and the shared test suite properly covers the new behavior. All adapter changes are narrowly scoped: the two GitHub search paths return empty maps instead of throwing, the GitHub status update now propagates API errors, the GitLab search returns the expected envelope shape, and the Gitea statuses normalization maps the API field names correctly. The test consolidation is consistent — adapters that don't support a feature explicitly skip the promoted tests. No data-loss, auth, or contract-breaking issues were found. The happy path of Important Files Changed
Reviews (4): Last reviewed commit: "fix: restore lost test coverage identifi..." | 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
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