test: assert what a provider refuses instead of skipping - #138
test: assert what a provider refuses instead of skipping#138HarshMN2345 wants to merge 1 commit into
Conversation
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 four. GitLab's test class keeps no test of its own, and Bitbucket only what only Bitbucket does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks for contributing! This repository is a read-only mirror; development for this library happens in |
Greptile SummaryThis PR strengthens the shared adapter suite by asserting unsupported capabilities and provider-specific responses instead of broadly skipping tests. It also normalizes Gitea-family pull-request update actions, corrects GitHub user lookup response handling, standardizes unsupported-operation messages, and makes GitLab repository lookups reject redirects.
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking weakness in how the new refusal tests validate exception messages. The runtime adapter changes are consistent with existing contracts and consumers; the only accepted concern is that the new test helper can produce false confidence by accepting refusal messages for the wrong method or provider. Files Needing Attention: tests/VCS/Base.php Important Files Changed
Prompt To Fix All With AI### Issue 1
tests/VCS/Base.php:376
**Refusal Check Is Too Broad**
`assertRefused()` is intended to verify the exact refusal message, but this pattern accepts any method name, any provider name, and arbitrary trailing text. For example, `getPullRequest() is not supported by github` could satisfy a `createPullRequest()` check. This can let the shared capability tests pass without enforcing the contract introduced here. Pass the expected method into the helper and compare the complete message, including `$this->vcsAdapter->getName()`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "test: assert what a provider refuses ins..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| protected function assertRefused(callable $call): void | ||
| { | ||
| $this->expectException(Exception::class); | ||
| $this->expectExceptionMessageMatches('/^\w+\(\) is not supported by /'); |
There was a problem hiding this comment.
assertRefused() is intended to verify the exact refusal message, but this pattern accepts any method name, any provider name, and arbitrary trailing text. For example, getPullRequest() is not supported by github could satisfy a createPullRequest() check. This can let the shared capability tests pass without enforcing the contract introduced here. Pass the expected method into the helper and compare the complete message, including $this->vcsAdapter->getName().
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/VCS/Base.php
Line: 376
Comment:
**Refusal Check Is Too Broad**
`assertRefused()` is intended to verify the exact refusal message, but this pattern accepts any method name, any provider name, and arbitrary trailing text. For example, `getPullRequest() is not supported by github` could satisfy a `createPullRequest()` check. This can let the shared capability tests pass without enforcing the contract introduced here. Pass the expected method into the helper and compare the complete message, including `$this->vcsAdapter->getName()`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
1 participant
Follow-up to #126. That PR moved every non provider specific test into
Baseand declared what differs. This one closes the gap it left: a declared capability dropped the test that needed it, so$supportsTags = falsemeant nobody ever checked what the adapter does when asked for a tag.Now a capability narrows a test instead of removing it. Where the adapter documents a refusal, the shared test asserts it; where the provider answers differently, the test asserts that answer.
Refusals are asserted, not skipped
assertRefused()requires the message theGitdefaults already use,X() is not supported by <name>, so a 404 from a repository that never existed cannot pass for a refusal. Eight capabilities assert their refusal: repository languages, commit statuses and their lookup, pull request creation and lookup, tags, check runs, namespaces, and installation repositories.GitHub's three stubs said
Not implemented; Gogs's seven saidPull request API is not supported by Gogs. Both now name the method, which is what makes the assertion meaningful.Skips that were hiding an answer
testListBranchesEmptyRepository[main]— the adapter asks for an initial committestListTagsCommitlessRepository[], which holds either waytestGetCommitAuthorAvatar''assertCommitAuthorLinks'', so a provider that starts reporting one flips the flagtestGetOwnerNameWith{out,Zero,Null}RepositoryIdtestGenerateCloneCommandWithTagtestGetUserWithInvalidUsernametestGetRepositoryAfterDeleteFailsassertIsArrayon a method typed: array[]Adapter changes
Sharing a test meant running it against a provider for the first time, which is where these came from.
synchronizedfor a pushed head. Every consumer acts onsynchronize— Appwrite's four VCS event handlers each test for it — so a pull request update on Gitea, Forgejo and Gogs was read as an action nobody handles. The adapter normalizes it; every other action passes through as sent.Exceptionfor a repository that does not exist, which is whyGitLabTesthad to declare$repositoryNotFoundException = \Exception::class.getRepositoryName()andgetOwnerName()now raiseRepositoryNotFoundon a 404, and the flag is gone.getRepository()reported a repository that had been deleted. It no longer follows that redirect, which also drops$deletesRepositoriesSynchronously.GitHub::getUser()sent no credential, the only call in the adapter that did not, and returned the whole response rather than the body — a 404 came back looking like a user.getInstallationRepository()was stubbed in three adapters with the wording each chose. Only GitHub models installations, so the refusal moved toGitbeside the other four.Tests hoisted out of the adapter classes
Two tests were provider specific only because the payload going in was.
GitLabTestandBitbucketTest, is now a declared$pullRequestActionsmap of native to shared name.Baseasserts every provider covers the three actions consumers act on and that nothing falls outside the vocabulary. Bitbucket names the action in the event rather than the payload, which one hook expresses.pushPayload()takes the commits to list before the head, andBaseasserts the head's hash, message, author and url.GitLabTestnow holds no test of its own.BitbucketTestkeeps the three that are genuinely Bitbucket: batched pushes, a linked commit author, and its repository shape.Also
Following the monorepo's test conventions:
finalon the leaf test classes,#[\Override]on real overrides, and thirteen cleanup blocks moved ontodiscardRepositories(), which retries and reports a repository it could not remove instead of leaving it to contaminate later runs.testGetDeletedRepositoryFailslooked up a repository that was never deleted; it is nowtestGetNonExistingRepositoryFails, beside the one that does delete.The adapter guide still told contributors to add tests to
tests/VCS/Adapter/VCSTest.php, which has never existed. It now describes the hooks, the flags, and what belongs in an adapter's own class.Verification
composer lintandcomposer check(PHPStan level 8) pass.Baseholds the same 102 tests; what changed is how many of them assert rather than skip. The suites are the real check, so CI is the verification.🤖 Generated with Claude Code