Add a mocked test suite covering every resource, and turn CI tests back on - #91
Merged
Conversation
…ck on
The test suite could not run in CI: it hit the live API, so it needed a
token that fork PRs do not have, and testServerRestart() restarts a real
server. The teststep in php_test.yml had been commented out because of it,
which meant nothing but PHPStan guarded the SDK.
There was also no seam to intercept HTTP. Ploi::setApiToken() builds the
Guzzle client internally and $guzzle is private, so no resource could be
tested offline.
Ploi::setHandler() now accepts a Guzzle handler, which is also useful in
its own right for retry or logging middleware. Tests plug a MockHandler
into it. Because the client is still assembled by setApiToken(), the base
URI, headers and http_errors setting under test are the ones users get.
Tests are split into two suites:
tests/Unit mocked, no network, runs by default and in CI
tests/Integration the existing live-API tests, opt-in via composer
test:integration, still needs tests/.env
Every one of the 36 resource classes now has a unit test asserting the
URL, the HTTP verb and the JSON body of each of its methods, plus the
RequiresId paths. HistoryTest never needed the network and moved to the
unit suite.
288 tests, 782 assertions, no network.
Adds tests/ to the PHPStan paths so the test suite is held to the same level 5 as src. The nine errors that surfaced: Resource::setId() and setIdOrFail() were declared ": self", which resolves to Resource rather than the called class, so chaining off them lost the concrete type. A "@return static" docblock fixes it at the source, which also helps anyone running static analysis against the SDK. Two of the findings were real bugs in the live tests: SiteTest::testCreateExampleDotCom() dropped the result of its own recursive call and then fell off the end of a method declared to return stdClass, so the @Depends chain got null after cleaning up a leftover site. Its $foundSite flag was also dead - never set true, so the break was unreachable. SshKeyTest::testDeleteSshKey() wrapped its only assertion in "if (!empty($sshKey))" on a parameter typed stdClass, which can never be empty. Harmless, but it hid that the guard did nothing. The rest were assertions that could not fail: assertIsArray() on Response::toArray() and on getHistory(), both of which are declared to return arrays. Replaced with assertions that check something.
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
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.
Why
The test suite was not running in CI. The step in
.github/workflows/php_test.ymlhad been commented out, because the tests hit the live API: fork PRs have no token, andtestServerRestart()restarts a real server. In practice only PHPStan was guarding the SDK.There was also no way to intercept HTTP.
Ploi::setApiToken()builds the Guzzle client internally and$guzzleis private, so no resource could be tested without a network call.The seam
Ploi::setHandler(?callable $handler)accepts a Guzzle handler. Useful on its own for retry or logging middleware, and it is what lets the tests plug in aMockHandler.The client is still assembled by
setApiToken(), so the base URI, headers andhttp_errorssetting under test are the ones users actually get — the tests are not asserting against a hand-rolled client. Production behaviour is unchanged when no handler is set.Two suites
tests/Unitcomposer test, and CItests/Integrationcomposer test:integrationtests/.envwith a real tokenphpunit.xmlsetsdefaultTestSuite="unit", so a barephpunitno longer reaches for the network. The four existing live tests moved totests/Integrationunchanged apart from their namespace — they still have value as a check that the real API matches.HistoryTestnever needed the network and moved to the unit suite.Coverage
All 36 resource classes have a unit test. Per method it asserts the full URL, the HTTP verb and the decoded JSON body, plus the
RequiresIdpaths:Tests\Unit\TestCaseprovidesqueue(),queueRaw(),queueMany(),request(),assertRequest(),assertRequestCount()andassertNoBody().PloiTestcovers the client itself: auth headers, base URI resolution, the supported verbs and every status-code-to-exception mapping.ResponseTestcoversgetJson()/getData()/toArray().288 tests, 782 assertions, zero network calls, ~25ms.
PHPStan now covers the tests too
phpstan.neonpicks uptests/alongsidesrc/, at the same level 5. Fixing what that surfaced turned up two real bugs in the live tests:SiteTest::testCreateExampleDotCom()discarded the result of its own recursive call and then fell off the end of a method declared: stdClass, so the@dependschain received null whenever it had to clean up a leftover site first. Its$foundSiteflag was dead code — never set to true, so thebreakwas unreachable.SshKeyTest::testDeleteSshKey()wrapped its only assertion inif (!empty($sshKey))on a parameter typedstdClass, which can never be empty.Resource::setId()andsetIdOrFail()were declared: self, which resolves toResourcerather than the called class, so chaining off them lost the concrete type. They now carry@return static, which also helps anyone running static analysis against the SDK.The remaining findings were assertions that could not fail —
assertIsArray()onResponse::toArray()and ongetHistory(), both declared to return arrays — replaced with assertions that check something.Verification
composer test→OK (288 tests, 782 assertions)vendor/bin/phpstan analyse -c phpstan.neon(nowsrc+tests) →[OK] No errorsvendor/bin/phpcs --standard=PSR12 src→ 130 errors, byte-identical to themasterbaseline; none addedcomposer validate --strict→ valid