T19 add rest test case#20
Merged
Merged
Conversation
Talon was the only project without ordered_class_elements; vokuro, invo, phalcon, cphalcon, volt and phql all carry a byte-identical block. Its absence is why class members here were never ordered by visibility and name the way the standard describes. declare_strict_types and no_unused_imports are kept as a local addition - phpcs (PSR-12) checks neither, so dropping them to match the shared set exactly would leave both unenforced. They should move into the shared set once the global standard is agreed.
Pure formatting: no behaviour changes. This is the one-off cost of adding the rule - every subsequent change is kept in order by composer cs-fixer-fix rather than by hand. Reordering moves members past each other, which leaves PSR-12 blank-line artifacts that php-cs-fixer does not own; phpcbf (composer cs-fix) settled those. Run the two in that order after any future reorder.
The REST surface needs a base URL. It goes in the extra bag rather than services because the fromEnv() service table is host/port-shaped and a URL does not fit it. CODECEPTION_URL/CODECEPTION_PORT are deliberately not carried over.
Both are needed by Symfony\Component\BrowserKit\HttpBrowser, which the REST surface uses to make real requests. browser-kit lists them as require-dev, not require, so neither arrives transitively. Mime is not only for file uploads: HttpBrowser::getBodyAndExtraHeaders() raises 'You cannot pass non-empty bodies as the Mime component is not installed' for every method other than GET/HEAD, and it checks that before it looks at the body at all - so even a PUT with no parameters fails without it. Constrained to ^6.4 || ^7.0 to match browser-kit and dom-crawler. Symfony 7.x requires PHP 8.2, so on our 8.1 floor composer resolves the 6.4 LTS line. psr/container, psr/log, deprecation-contracts and service-contracts move from packages-dev to packages at unchanged versions, because http-client requires them at runtime.
getDescription() returns the '404 (Not Found)' form. It is deliberately an independent implementation of the standard reason phrases rather than a lookup into the application under test: rest-api emits that exact string in its JSON error payload from its own table, so an assertion comparing its output against its own source would assert nothing.
Keys and list elements present in the response but absent from the expectation are ignored, so a fragment can be asserted against a full JSON:API envelope. List elements match in any order; scalars compare strictly.
Validates a decoded document against a map of types rather than values: string, integer, float, boolean, array, null, the :date filter, | unions, and nested maps. Keys absent from the map are ignored - the envelope check names only jsonapi and meta while the document also carries data. Types are strict, so an int does not satisfy float. Scoped to what is needed plus the obvious neighbours; Codeception's JsonType carries a broader DSL that is not worth cloning.
Drives a BrowserKit HttpBrowser over symfony/http-client, so requests cross
a real HTTP boundary. The in-process Browser\Client cannot serve here: it
never maps request headers into $_SERVER (rest-api reads its JWT via
getHeader('Authorization')), and a raw JSON body cannot reach php://input
in-process.
restHttpClient() is the transport seam. It returns null so HttpBrowser
builds a real client; a test overrides it with a MockHttpClient and drives
the same request-building path without a live server.
Request headers are tracked in the trait and re-pushed through
setServerParameters() on every change, because BrowserKit has no
removeServerParameter() - rebuilding the whole set is the only way to drop
one, which unsetHttpHeader() needs.
$files is not carried over from Codeception's signature: rest-api uploads
nothing and no use case needs it yet.
Mirrors the BrowserTrait/BrowserAssertionsTrait/AbstractBrowserTestCase split: actions read as verbs, assertions are assertX, and the assertions trait reaches the response through abstract declarations that RestTrait satisfies. AbstractRestTestCase needs no $_SESSION isolation, unlike AbstractBrowserTestCase - requests cross a real HTTP boundary, so there is no in-process session to leak between tests.
Coverage was already 100%, but coverage only proves a line ran - infection showed 84 mutants surviving in the new code. The largest group was PublicVisibility: reducing a public assertion to protected broke nothing, because the tests called it via $this from inside the class. Fakes/Rest/PublicApiConsumer follows the existing Fakes/Browser/PublicApiConsumer precedent and exercises the surface from outside the hierarchy, which is what actually pins the visibility. DefaultSeamConsumer exists separately because PublicApiConsumer overrides both seams, and an override masks whether the trait's own definitions are still reachable from a child. The rest were tests asserting only the happy path: removing the inner assertion of an assert*() left every test passing, and the range bounds were never pinned from outside (a test using 404 cannot tell 400..499 from 401..498). Each assertion now has a failing case, and each bound is pinned from both sides.
Two changes. The line references for Settings::getDatabaseDsn were stale: applying ordered_class_elements moved the method from 186 to 246, so the MatchArmRemoval and Throw_ ignores pointed at unrelated lines. Any line-scoped ignore in a file the reorder touched had to be rechecked; these two were the only ones that moved. Worth knowing for next time - line-scoped ignores do not survive reordering, which is why the new entries below are method-scoped. Four new ignores for genuinely unkillable mutants, each unobservable from a test rather than merely inconvenient to cover: a break that only short-circuits a scan, an explode() limit the DSL can never reach, and two casts that are no-ops on every path that can observe them.
infection requires thecodingmachine/safe at dev-master, which autoloads 80 files eagerly - they land in vendor/composer/autoload_files.php, so they are parsed on every require of the autoloader, not only when infection runs. On PHP 8.5 that emits deprecations across the whole matrix rather than just the one cell mutation testing uses. The lock happened to pin safe at 4fbc008, an older dev-master commit that still behaves, so this had not surfaced here yet. dev-master is a moving target and dependabot is active, so it was a matter of time. resources/infection.json5 and the test-mutation script stay, as does the extension-installer allow-plugin entry, so the setup comes back with a single composer require --dev. The CI step is commented out rather than deleted for the same reason. Mutation testing was run before this removal: MSI 99%, with no escaped mutants in the REST surface. Revisit once the minimum PHP version moves past 8.1 and infection can be brought back.
Raw string bodies went out as text/plain. HttpBrowser wraps an unlabelled string body in a TextPart, so sendPost($url, json_encode($data)) - the most natural call on this surface - silently mislabelled every request. String bodies now default to application/json, per request, and only when the caller has not named a type. The test asserted only the body, which is how a path at 100% coverage and 99% MSI still shipped wrong: the line ran and the mutants died, but nothing looked at the header. Request headers now ride the per-request $server argument instead of setServerParameters(). BrowserKit merges that over its own bag rather than replacing it, so a caller's setServerParameter() survives. The old approach rebuilt the whole bag on every header change and silently dropped anything it had not set - aftermath of needing an unset that BrowserKit has no API for. syncRequestHeaders() is gone with it, and so is the ordering subtlety where restBrowser() had to sync after constructing. useRestBaseUrl() makes the base URL injectable rather than only overridable. It was reachable solely through Talon's process-global Settings, which is why PublicApiConsumer had to override the seam - and that override is what masked the visibility mutants and forced a second fake. $files is back, now that mime is a dependency and the reason for dropping it is void. It takes a plain path as well as the $_FILES shape: BrowserKit's answer to a plain path is to stop reading the list and send nothing, silently, which is a poor thing to hand someone.
['data' => []] asserted nothing. Read as a pure subset an empty list matches
every document, so the fragment only proved the key existed - and rest-api's
seeSuccessJsonResponse() defaults straight into that shape, so the conversion
would have inherited a suite of assertions that could not fail. An empty
expected list now means empty. An empty fragment at the top level is rejected
outright, since it can only be a caller mistake.
'float' rejected whole numbers. JSON has a single number type, so json_decode
returns int for {"price": 10} and float for {"price": 10.5} - the same field
is one or the other depending on the value it carries, and a strict 'float'
fails on every round one. It accepts both now; 'integer' stays strict.
Two entries had gone stale against the code: JsonType claimed strict types (float now accepts a whole number) and JsonSubset claimed absent list elements are always ignored (an empty expected list now means empty). File uploads and useRestBaseUrl() were added after the entries were written and were missing.
match() scored 17 against Sonar's rules, over the default threshold of 15. It
was traversing the map, checking key presence, dispatching leaf-vs-nested and
formatting error text all at once, and the nesting penalty did most of the
damage - two branches cost +3 each purely for sitting two levels deep.
The tell was the guard reading
!is_string($expected) || !self::matchesSpec($value, $expected)
which collapsed two distinct failures into one branch, then re-evaluated
is_string() inside the sprintf to work out which one it had caught. matchValue()
keeps them apart so each reports itself. path() and typeError() take the
remaining inline work out of the loop, and the continue goes with it.
match() is now 6; nothing in the file is above it. Behaviour is unchanged - the
existing tests carried the refactor untouched.
Mutation testing also showed 'float' accepting a string, because negating both
operands of is_float() || is_int() is unobservable without a non-number case.
The upload tests uploaded __FILE__, so the multipart body embedded the test's own source - which contains every string the assertions look for. They passed whatever was actually sent, including when only the first of several files was. They now post a fixture whose contents cannot collide, and assert on the Content-Disposition filenames. The form-body test asserted 'username' and 'sarah' appear in the body; both appear in a JSON body too, so it could not tell the form path from the JSON one. It pins the urlencoded body now. useRestBaseUrl() had no caller outside the class hierarchy, so nothing held its visibility. The empty-fragment message was asserted by a pattern matching only its first half; it is one literal now, with nothing to reorder or drop.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Code Metrics Report
Details | | master (ee35373) | #20 (6b856eb) | +/- |
|---------------------|------------------|---------------|------|
| Coverage | 100.0% | 100.0% | 0.0% |
| Files | 46 | 52 | +6 |
| Lines | 777 | 959 | +182 |
+ | Covered | 777 | 959 | +182 |
- | Test Execution Time | 1s | 2s | +1s |Code coverage of files in pull request scope (100.0% → 100.0%)
Reported by octocov |
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
1 participant
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.
Hello!
In raising this pull request, I confirm the following:
Added a REST/JSON test surface:
Phalcon\Talon\Traits\RestTrait(the full verb set, file uploads, request headers that persist across requests,amBearerAuthenticated()/amHttpAuthenticated(), redirect control, and response grabbers),Phalcon\Talon\Traits\RestAssertionsTrait(status, range, body, header, and JSON assertions), andPhalcon\Talon\PHPUnit\AbstractRestTestCasewhich composes both. A raw string body is sent asapplication/jsonunless a content type is set, and$filestakes either a plain path or the$_FILESshape.Added
Phalcon\Talon\Http\HttpCode- HTTP status constants plusgetDescription(), which returns the404 (Not Found)form. It is deliberately an independent implementation of the standard reason phrases rather than a lookup into the application under test, so that asserting an application's emitted status string against it actually asserts something.Added
Phalcon\Talon\Http\JsonType- validates a decoded JSON document against a map of type expectations (string,integer,float,boolean,array,null, the:datefilter,|unions, and nested maps). Keys absent from the map are ignored, so a map can describe just the part of an envelope a test cares about.floataccepts a whole number too - JSON has a single number type, so{"price": 10}decodes to an int and a strictfloatwould fail on every round value.integerstays strict.Added
Phalcon\Talon\Http\JsonSubset- recursive subset matching, so a fragment can be asserted against a full document. Keys and list elements present in the response but absent from the expectation are ignored, and list elements match in any order. An empty expected list is the exception:['data' => []]asserts thatdatais empty rather than matching anydataat all.Added
TALON_REST_URLtoSettings::fromEnv(), readable viaSettings::get('rest_url')and defaulting tohttp://127.0.0.1:8080. It is resolved through Talon's sharedSettingsand so is the same for every test in a run;RestTrait::useRestBaseUrl()points an individual test somewhere else.Thanks