test: trace the corpus to the matrix and close coverage gaps - #342
Merged
Conversation
Conflict in the conformance corpus table: this branch added a rows field naming the matrix rows each spec witnesses, main added four specs (discriminator-transitive, nullability-conjunction, path-item-docs, path-item-operations). Kept both — the four now name their rows: transitive discrimination witnesses tagged-unions and inheritance, the nullability conjunction witnesses optionality-vs-nullability, path item docs witness docs-summary-description, and the 3.2 operation fields witness http-binding.
The corpus-to-matrix checks added on this branch had several ways to pass while the thing they describe was broken. The parameter-style completeness check counted deduplicated sets, so any parameter shadowed by another resolving the same way could be deleted from both the fixture and the expectation undetected — every location-defaulting row among them, including the cookie row the fixture was written for. With it gone, the regression that motivated the fixture went green again. The expectation is now generated from the legal (in, style) table rather than typed out beside the fixture, so the only way to stop demanding a parameter is to delete the pair OpenAPI allows. The matrix reader ended the table at the first line not starting with a pipe, so a blank line or an indented row dropped that row and every row below it out of every check. It now requires the document to hold no table-shaped line outside the table, matches the header on parsed cells instead of raw text, and treats only a backslash-pipe as an escape. Six specs named capability rows their goldens do not show: a spec whose subject is keywords with no IR home claimed constraints, one whose format keyword lowers to a primitive claimed encoding hints, and so on. All six rows were witnessed elsewhere, so nothing went red. The pagination row had only a false witness and is now listed as uncovered. Also: a golden pins each row key to the capability it labels, so a column shift or a rename of a row no spec names is a diff; the legend guard covers all nine format columns rather than one; an excuse for a row OpenAPI cannot express is now rejected instead of living forever; the parameter map is keyed on (name, location) as OpenAPI keys it; and the querystring parameter moves to its own path item, since 3.2 forbids it sharing one with a query parameter. Two matrixRowsUncovered reasons pointed at an IR that could not hold the capability yet; ir.LongRunning and ir.Idempotency both model theirs, and the reasons now name the real blocker.
Generating the expectation from the legal (in, style) table stopped the fixture shrinking on its own, but not the table and the fixture shrinking together: deleting a pair, deleting the parameters it generated and regenerating the golden left every check agreeing with the smaller world. That is the same failure one level up, and the only thing outside the loop is the specification, so the table is now held to a count that is OpenAPI's. The two halves are held to each other as well — a location's default style must be a style that location legally takes. Also drops an unreachable branch from the matrix row reader, names both causes in the one-table check's failure message, and stops the parameter fixture claiming its neighbour covers nothing the table covers, which was not true of the neighbour's assertions.
Keying the reader on a leading pipe made it blind to a table GFM writes without outer pipes, and that blindness cost twice. A second keyed table written that way escaped the "exactly one table" guard and every check in the file, including the witness contract its rows claimed to be under. Dropping the outer pipes from the table's *last* row removed it from the contract while the row-count check still balanced, because the line stopped being counted on both sides at once — and one routine -update would have baked that in. Tables are now found by the delimiter row every markdown table has and prose does not, rows are recognized wherever the pipes sit, and fenced blocks are skipped so the document may hold a table as an example without becoming unreadable here. A table-shaped line outside the table is named with its line number rather than blamed on truncation. Cell splitting now treats \\ as GFM does. Reading only \| as an escape made the pipe in \\| look escaped when the backslash before it was what had been escaped, merging two cells into one. The three reasons in matrixRowsUncovered that defer to a policy pass now say what retires them. A reason with no retirement condition is the same failure as the stale ones this branch already rewrote, written the other way round: it describes a gap that reads as closable and is not. Also makes the parameter-name builder total instead of slicing a style that the neighbouring comment invites writing empty, rejects such a pair where it can be named, and drops two guards BodyTarget performs itself.
…rose Two ways the reader called an edit broken when it was not. A fenced block was skipped only when the fence was written with backticks, so the same table example written with tildes — CommonMark gives the two the same meaning — was read as a second table and failed four tests. Both spellings now count. A line outside the table was reported whenever it held a pipe anywhere, so a sentence naming the `|` character failed those same four tests with nothing to say about why. A line is now reported only when it splits into as many cells as the header, which is what a row that fell out of the table looks like and what prose does not. Both detections that check guards are unaffected: a row moved out of the table and a second table written without outer pipes each still fail.
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.
Summary
docs/ir-spec-matrix.mdstates the corpus contract — one minimal spec per row per format thatcan express it — and nothing checked it. The 54 rows carried no identifier, no Go file opened the
document, and so no command could answer how many rows had a witnessing spec. Two rows that the
gap was hiding are fixed here as well.
Traceability. Every matrix row now carries a stable key, and each conformance case names the
keys it witnesses.
conformance_matrix_test.goparses the table and requires every row theOpenAPI column marks ✅ or ⚠ (42 of 54) to be witnessed by a spec or listed in
matrixRowsUncoveredwith a written reason: open enums, long-running operations, idempotency andpagination. The checks run both ways — a key spelled in Go must name a row the document declares,
a row marked absent for OpenAPI must have neither a witness nor an excuse, and a row listed
uncovered must have no witness, so the list cannot outlive the gap it describes. A golden pins
each key to the capability it labels, which is what stops the two columns drifting apart and what
extends "renaming a key is a diff" to the rows no spec names yet. This extends
TestConformance_TableNamesEveryCorpusSpec, which already ran the spec → table direction; thenew tests run row → spec.
What the tests deliberately do not claim is that a spec naming a row exercises that capability.
That is a reviewer's reading, and
docs/architecture.mdandCLAUDE.mdsay so rather thanimplying otherwise — six early row claims here were wrong in exactly that way (a spec whose whole
subject is keywords with no IR home claimed
constraints; one whoseformatkeyword lowers to aprimitive type rather than to
ir.Encodingclaimedencoding-hints), and every one of those rowswas witnessed elsewhere, so nothing went red.
paginationhad only a false witness and is nowlisted as uncovered instead.
Parameter styles. OpenAPI's Parameter object admits nine legal
(in, style)pairs thiscompiler can accept — path
simple|label|matrix, queryform|spaceDelimited|pipeDelimited|deepObject, headersimple, cookieform— plus a location,querystring, that takes no style at all. Four pairs appeared in a committed golden.param-style-matrix.yamldeclares every pair three times, writingexplodetrue, false andomitted, beside the four locations that omit
styleso the per-location default table is whatanswers.
The expectation is generated from that pair table rather than written out beside the fixture.
That is what makes a shrinking fixture visible: a literal expectation and a fixture are two copies
of one claim, and deleting a row from both leaves nothing to disagree — which is what a
set-counting check missed, since a deleted parameter still leaves its
(location, style, explode)triple behind whenever another parameter resolves the same way. Generating it moves that question
up rather than answering it, since the table and the fixture can still shrink in step, so the
table itself is held to a count that is OpenAPI's — 3.2 tabulates the legal combinations and says
"Combinations not represented in this table are not permitted". Changing that number is a claim
about the format rather than a quiet edit, and it is the only place a count is written down. The
map is keyed on
(name, location), as OpenAPI keys the Parameter object; keyed on the name aloneit silently drops one of a legal same-name pair.
3.2 forbids a
querystringparameter from sharing an operation, or that operation's path item,with any
in: queryparameter — in both directions — so it gets a path item of its own. #389 isfiled for the one legal pair no fixture can declare: 3.2's
cookiestyle atin: cookie, whichthis compiler rejects outright, together with the explode default 3.2 gives it.
Inline hoist positions. Six positions where an inline composite is hoisted had no
per-position assertion: a parameter's schema, a response header's schema, a property inside a
request body, a property inside a response body, a webhook operation's body, and a callback
operation's body. The callback body had no anonymous node anywhere in the committed goldens.
inline-hoist-positions.yamldeclares the same body at all six and pins each pointer-derived IDtogether with the site that refers to it.
A correction to issue #254 while closing it: it counts eleven uncovered positions out of twelve,
having looked only at
compilers/openapi/hoist_test.go.inlinePositions()incompilers/openapi/internal/schema/schema_test.gois a per-position table already assertingpointer-derived anonymous IDs, and it covers six of the twelve plus two the issue does not list.
The six above are the real gap, and they sit outside the schema package's reach, which is why the
corpus is where they land rather than that table.
Issue #334 is filed for a defect the new parameter fixture surfaced:
in: querystringcomes outcarrying
style: formandexplode: true, a style the specification does not allow at thatlocation. It is not fixed here — the current values are pinned with a comment citing the issue, so
closing it reddens the case rather than changing the IR unnoticed.
Test plan
./scripts/check-coverage.shand the rest of the gate pass. Each check was verified by plantingthe defect it exists to catch and watching it go red:
cookieDefaultedfrom the fixture, deleting any other parameter a pair generates, ordeleting the cookie pair from the legal-pair table. The first of these passed under the
set-counting check this replaces, and with it passing, dropping
soa.ParameterInCookiefromdefaultParamStylewent green again too.synchronized shrink the generated expectation does not by itself prevent. It fails on the pair
count. So do a duplicated pair and a location whose default style is not one of its legal pairs.
soa.ParameterInCookiefromdefaultParamStyle, and replacingexplode := style == formwithexplode := false.the pair table does not generate.
both of which the earlier reader accepted while silently dropping rows.
adding an expressible row with no witness, marking a witnessed row absent, witnessing a row
listed uncovered, garbling a legend marker in a non-OpenAPI column, and excusing a row OpenAPI
cannot express.
from the other each redden their case — so the goldens were regenerated, not merely rewritten.
(
/callbacks/,/webhooks/,/parameters/,/headers/,/requestBody/,/responses/),fails
TestConformance/inline-hoist-positionsat every one of the six./callbacks/previouslyfailed no conformance case at all.
reencodeMappingsand
reverseMappingsboth succeed and produce different bytes for each.Closes #71
Closes #253
Closes #254