Repository navigation
Test suite guardrails: real caching coverage, perf gate, concurrency, row-order, bug pins, manifest CI, streaming/cancellation - #335
Merged
Conversation
Build regenerates ManifestOutput goldens in place via QuarrySqlManifestPath; CI now fails when the regenerated manifests differ from the checked-in files, so generator SQL drift can no longer merge green unnoticed.
Adds a public TrackingNames constants class and applies WithTrackingName to the seven load-bearing pipeline nodes (context declarations, entity registry, raw/enriched/bound/translated call sites, per-file groups) so caching tests can assert per-stage run reasons instead of scanning unnamed output steps.
…ins (#314) Fixtures move from anonymous-type projections (which disqualify into RuntimeBuild/QRY032 and emit hollow interceptor shells) to named-tuple and single-column projections, with health guards asserting no generator errors or CS8785 and real SQL in every emitted interceptor. - Unchanged-run test re-parses identical text into fresh trees, forcing the driver through model equality instead of reference-equal short-circuits; named stages must report only Cached/Unchanged and interceptor text must be byte-identical. - Modify-one-file test asserts the changed file's per-file group regenerates while the sibling's interceptor text stays identical; the sibling group's Modified reason is pinned to #310 (output action mutates cached AssembledPlan.ReaderDelegateCode, which participates in equality). - Modify-query test compares captured interceptor text before/after. - New schema-only-edit test: registry rebuilds, entity output regains the new column, interceptors stay real. - New pin for #310 defect 1: cross-partial ordinal shift leaves a stale display-class name in incremental emission vs a clean run. generator fix (user-approved inline): DisplayClassEnricher.EnrichAll crashed with ArgumentException -> CS8785 (all interceptors silently dropped) when a persistent driver was re-fed identical source parsed into fresh trees, because cached RawCallSites hold nodes from superseded trees. EnrichAll now recovers the equivalent lambda from the current compilation by file path + span.
New IR/PipelineModelEqualityTests: EntityRegistry difference detection built directly from context fixtures (context-list regression pin for the shipped _allContexts omission, column-type and dialect variations), and AssembledPlan/CarrierPlan/FileInterceptorGroup coverage via real generator runs — identical independent runs must produce an equal graph with equal hashes; WHERE-literal, projection, captured-variable, and file-path variants must each break equality in the constituent that owns the change.
(#314) Empirical findings reshaped this step (details in issues #328/#329): - The historical conditional-Having context misattribution no longer reproduces — the stale NOTE in CrossDialectConditionalMaskTests is replaced by a real taken-branch regression test (per-dialect quoting doubles as the attribution assertion). - What does reproduce: the conditionally-applied Having clause is not mask-gated and renders unconditionally. #328 was retitled to that defect and the untaken-branch pin (KnownBug_Issue328_...) asserts the buggy behavior — it fails exactly when the bug is fixed. - #329 (entity-terminal interceptor emitted as IQueryBuilder<T,T> -> CS9144 build error) is confirmed real; all integration-suite .Select workaround comments now reference #329. No compilable pin is possible for a build error; the guard matrix lands in the next step.
…#314) Plan step 7 (F5c). Adds Generation/InterceptorBindingGuardTests.cs: 18 chain shapes — entity terminals (no explicit .Select), generic terminals on generic receivers (ExecuteScalarAsync<TKey>), and modification terminals — each built in its own synthetic compilation with interceptors enabled, asserting no CS8785/CS9144/CS9177 and that the terminal's interceptor was actually emitted. The 9 entity-terminal shapes re-run against a second context in a nested namespace. Removes <NoWarn>CS9177</NoWarn> from Quarry.Tests.csproj: a non-incremental build without it reports 0 errors and zero CS9177/CS9144, confirming the step-6 finding that the suppression was vestigial and mislabelled. Also recovers a compilable pin for #329, which step 6 concluded was not pinnable. The emitter does reproduce the defect in isolation — an entity-terminal chain gets an interceptor typed IQueryBuilder<T, T> for an IQueryBuilder<T> receiver — it just raises no CS9144 there, which is why the earlier probe read as correct. KnownBug_Issue329_EntityTerminal_EmitsTwoArityReceiver pins the emitted text, so fixing #329 fails the pin and signals that the .Select(...) workarounds in the integration suites can go. Suite: 3474 Quarry.Tests / 201 Migration / 146 Analyzers, all passing.
Plan step 8 (F3). Adds Integration/ConcurrencyTests.cs — the suite previously never executed two Quarry operations at once, so the thread-safety of carrier statics, compiled-SQL fields, reader/mapper caches and the OpId counter was only ever correct by construction. Three tests, 8 workers each: - mixed SELECT/UPDATE/Patch, every worker driving identical chain shapes with its own parameter values and asserting it reads back its own — a shared parameter buffer would show up as cross-talk; - barrier-synchronized first touch of one shared carrier chain, so static initialization is genuinely contended; - independent contexts on separate connections querying all four dialects. Writes are SQLite-only: each harness owns a private in-memory database, while the container dialects share one seeded baseline behind a per-harness transaction, where concurrent writes would contend on row locks and yield timeouts rather than findings. Those dialects are exercised read-only. Harnesses are created sequentially so the test races Quarry, not the fixtures. Worker bodies are named methods rather than inline lambdas: a chain written inside a lambda nested in another lambda makes the generator emit interceptors referencing display-class locals, which fails to compile with CS0103. Recorded in workflow.md for follow-up. Suite: 3477 Quarry.Tests / 201 Migration / 146 Analyzers, all passing.
Plan step 9 (F7a). The iterator disposes its command and reader through `await using` declarations, but `FinalizeQuery` only runs on natural completion, so the abandonment path had no coverage — the one existing early-break test asserts what the consumer sees, not that anything was released. Adds three tests to CrossDialectStreamingTests, all four dialects: - abandon via `break` after the first row, then run a follow-up query on the same harness connection; - same abandonment via an explicitly disposed enumerator; - tear the harness down with streams abandoned on every dialect and assert the rollbacks still complete. The follow-up query is the assertion that matters: the harness holds one long-lived connection per dialect, so a leaked reader poisons it. Verified by removing `await using` from the reader in both streaming overloads — the two follow-up-query tests fail, confirming they detect a real leak. The rollback test keeps passing under that mutation (providers tolerate a rollback with a reader outstanding), so its doc comment now states plainly that it guards teardown rather than disposal. Manifest goldens regenerated for the new chains. Suite: 3480 Quarry.Tests / 201 Migration / 146 Analyzers, all passing.
Plan step 10 (F7b). Every terminal accepts a CancellationToken and every executor deliberately excludes OperationCanceledException from the catch that wraps failures, but none of it was exercised — the only tokens in the suite were CancellationToken.None placeholders and generator-side signature checks. Adds Integration/CancellationTests.cs: - pre-cancelled token into every fetch terminal (FetchAll/First/FirstOrDefault/ Single/SingleOrDefault/Scalar/NonQuery) on SQLite and PostgreSQL, plus the list terminal on MySQL and SQL Server; - mid-stream cancellation leaving the connection usable on all four dialects; - mid-stream cancellation surfacing OCE, asserted on SQLite; - pre-cancelled token into the raw-SQL streaming overload, whose token sits ahead of a params array and is easy to get wrong at a call site. Each test runs a normal query afterwards on the same connection, since a terminal that abandons a reader on the cancellation path would poison the harness's long-lived connection. Mid-stream OCE is asserted on SQLite alone on purpose: with three seeded rows PostgreSQL returns the whole result set in one response, so the read loop never awaits again and never observes the token. Forcing the container dialects to stream would take bulk inserts through four dialect-specific statements, which buys little over the connection-usability check. Documented in the test and in workflow.md. Verified by dropping the token in QueryExecutor (ExecuteReaderAsync and ReadAsync): both OCE assertions fail, confirming they detect a token that stops being threaded through. Suite: 3484 Quarry.Tests / 201 Migration / 146 Analyzers, all passing.
…es (#314) PostgreSQL, MySQL InnoDB and SQL Server do not guarantee row order without a top-level ORDER BY, so 87 positional assertions on pg/my/ss result lists across 29 tests passed only by accident of the planner. Route those fetches through SortedByAsync with a key that is total over the seeded data; the SQLite side stays positional as the reference shape. Not converted, and deferred to the query-side pass: nine JoinTests that encode orders.OrderId ordering through a column they never project, and four LIMIT/ OFFSET pagination tests whose row *selection* is nondeterministic.
Benchmarks previously published with no threshold or alert logic at all, so a regression only surfaced if somebody happened to read the dashboard. Compare each run against the last published entry in the benchmarks repo data.js: a mean regression over 15%, or any increase in allocated bytes, on a Quarry_* series now opens a deduped GitHub issue. Perf stays alert-only — a noisy benchmark box that reddens the workflow trains people to ignore it. A *missing* series is treated differently and does fail: that means a benchmark crashed or was dropped by the existing select(.Statistics != null) filter, which is an infrastructure failure rather than a performance judgement. Both checks run before the publish steps so a broken run never becomes the stored baseline.
#314) Second file group of the row-order sweep. Twenty-one sites in WhereTests and three missed by the earlier partial pass in CteTests now go through SortedByAsync. WideTupleTests and SetOperationTests needed no conversions, for opposite reasons worth recording: every WideTuple test already carries a top-level ORDER BY (sorting there would mask a dropped ORDER BY — one of them pins a DESC ordering that an ascending key would coincidentally satisfy), and SetOperationTests was already hardened with inline client-side sorts. Also folds the three ad-hoc (await ...).OrderBy(k).ToList() fetch lines in Tuple_PostCteWideProjection into SortedByAsync, which is what the helper's own doc comment asks for.
Final file group of the row-order sweep: 31 sites across EntityReader, HasManyThrough, NullableValue, FkKeyProjection and Prepare tests, including six FkKeyProjection fetch lines folded from the ad-hoc (await ...).OrderBy(k).ToList() form into SortedByAsync. Sixteen of the twenty-one remaining files needed no change, and for reasons worth stating: StringOp/Misc/Aggregate and Composition/WindowFunction assert count-only, .All(...) or .First(predicate) throughout; ConditionalMask carries an unconditional OrderBy in every chain; OrderBy/DistinctOrderBy pin a top-level ORDER BY that a client-side sort would mask.
Six tests picked an arbitrary subset with no ORDER BY, which no client-side sort can repair: the four LIMIT/OFFSET pagination tests, an ExecuteFetchFirstAsync over a two-row predicate, and a Limit(1) in WhereTests. Each chain now carries a query-side OrderBy and the expected SQL is updated to match. SQL Server gains real coverage from this: ORDER BY (SELECT NULL) satisfies the T-SQL grammar for OFFSET/FETCH but imposes no ordering, so those tests were asserting an order the query never promised. Parameter indices are unchanged, so the mixed literal/parameterized tests still pin the same index assignment. Where_CollectionPlusScalar_WithPagination asserted only a count, so it also gains a value assertion. The nine CrossDialectJoinTests that encode orders.OrderId ordering through a column they never project are tracked in #332 instead of being changed here; a remarks block on the fixture records why a composite key would silently reorder the assertions. Manifest goldens regenerated.
…314) llm-testing.md gains a row-order section covering SortedByAsync, why the SQLite side stays positional, what makes a key total, and the two cases a client-side sort cannot reach; a runtime-behaviour section for the concurrency, streaming-disposal and cancellation suites with the traps that shaped each; the KnownBug_Issue{N} pinning convention; and CI enforcement of the manifest goldens. Three gotchas added: chains in doubly-nested lambdas, partial chains passed as method arguments, and the QRY033 Prepare() rule. src/Quarry.Generator/llm.md documents the TrackingNames constants and the two Roslyn semantics that decide whether a caching assertion means anything -- an absent stage is itself a cached signal, and reference-equal compilations never invoke model Equals.
The rebase conflict in the four goldens was resolved by taking this branch's side, which drops the CTE entries added by #306 upstream. Goldens are build output, so the fix is a rebuild rather than a hand-merge.
Fifteen A items and twelve B items from review.md. The one that mattered: asserting that the interceptor-binding guard fixtures compile cleanly (previously they were only filtered for CS8785/CS9144/CS9177) failed immediately on both InsertBatch shapes. TerminalBodyEmitter hard-codes a call to internal Quarry.Internal.BatchInsertSqlBuilder, so InsertBatch does not compile for any consumer outside Quarry's InternalsVisibleTo list. Every in-repo project that uses it happens to hold a grant, so nothing had exercised it as an ordinary consumer would. Filed as #334 and pinned; the two shapes move out of the clean-binding matrix, their arity coverage retained by the plain Insert shapes. Benchmark workflow: issue filing moves to a separate ubuntu-latest job that consumes only the perf-gate artifact, so issues: write is no longer reachable from project and NuGet code on the persistent self-hosted runner. That job carries continue-on-error, restoring the alert-only intent an API blip could previously violate by reddening the run and skipping the publish. An open issue now gets a comment rather than a silent skip, since skipping plus an advancing baseline meant one issue silenced the channel permanently. A missing series gains an allow_missing_series override so a deliberate rename cannot red-lock the pipeline. CI manifest gate: the goldens are deleted before the build, so the check also proves they were regenerated rather than merely unchanged; untracked goldens are caught too. .gitattributes keeps the comparison platform-independent. Also: vacuous-on-empty stage assertion, an unscoped #329 pin, a barrier with no timeout, harness leaks on partial creation failure, three diverged reference lists, and per-field equality coverage for fields no test varied. Filed #333 for the CS0103 nested-lambda defect found in step 8.
…rap (#314) Workers 8 -> 4. Four is enough for worker-to-worker cross-talk to be observable and holds half as many connections against the shared baseline. ParallelFirstTouch is kept. It was briefly removed on two arguments that were both wrong, and the correction is worth recording: its cost was a measurement error, and the claim that NUnit's fixture ordering makes the first-touch premise unreliable was overstated -- the chain shape is unique to that test, so no other test can have initialized that carrier. The measurement error is the useful part. Filtering to one container fixture makes it the first consumer of EnsureBaselineAsync, so it absorbs the entire Docker boot: the concurrency fixture reads as 26s alone and 0.22s in a full run. In a full run that ~34s lands on MidStreamCancellation purely because it is the first container-touching test by start order -- the next one takes 0.02s. llm-testing.md now says so, and says to use a TRX logger for per-test durations rather than trusting console totals or filtered runs.
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
Addresses all seven findings from the 2026-07-07 multi-agent deep review (tests perspective): hollow
incremental-caching tests, no perf regression gate, zero concurrency testing, row-order flakes,
known generator bugs routed around instead of pinned, unenforced manifest goldens, and untested
streaming/cancellation. The two bundled low items (test parallelization, display-class canary) were
deferred by decision — parallelization is orthogonal and risky, the canary belongs to #310.
Suite goes from 3424 to 3501 tests, all green (plus Migration 201, Analyzers 146).
Reason for Change
The suite's weak spots were not missing tests so much as tests that could not fail.
IncrementalCachingTestsexercised anonymous-type projections, which are disqualified to
RuntimeBuild— so every interceptor itinspected was a hollow shell, and its "unchanged run" case reused the same
CSharpCompilationinstance, somodel
.Equalswas never invoked at all. 526 positional row assertions on PostgreSQL/MySQL/SQL Server ranagainst queries with no
ORDER BY. The manifest goldens had no CI enforcement. Benchmarks published with nothreshold logic, and silently dropped failed runs from the dashboard.
Impact
result:
DisplayClassEnricherdereferencing superseded syntax trees on a warm rebuild →CS8785 → every interceptor silently vanishes), found by the new fresh-tree unchanged-run test and fixed
inline with user approval;
InsertBatchinterceptors callinternalBatchInsertSqlBuilder, so the feature does notcompile for any consumer outside Quarry's
InternalsVisibleTolist. Found during review remediation, bythe assertion that guard fixtures must compile cleanly. Every in-repo project that uses
InsertBatchhappens to hold a grant, which is why nothing had exercised it as an ordinary consumer would;
(Conditional .Having(...) on a split GroupBy chain is not mask-gated — HAVING renders unconditionally #328 conditional
Havingnot mask-gated, Entity-terminal chains (no explicit Select) are silently not intercepted — CS9177 arity mismatch papered over by blanket NoWarn #329 entity-terminal receiver arity).CrossDialectJoinTeststhat encode row order through a column they never project;left unchanged by decision, with a
<remarks>block explaining why a plausible-looking composite sort keysilently reorders them.
Plan items implemented as specified
WithTrackingNameon seven load-bearing pipeline nodes, names as typed constantsawait usingfrom the readerORDER BYllm-testing.mdandsrc/Quarry.Generator/llm.mdupdatedDeviations from plan implemented
approval (
DisplayClassEnricherrecovers the equivalent node from the current compilation byFilePath+ span). Two assertions became Display-class prediction robustness: stale UnsafeAccessorType names on cross-partial ordinal shifts; generic containing types/methods unguarded (runtime TypeLoadException) #310 pins rather than correct-behaviour assertions,because the defects they hit are real and tracked.
real defect remains in the same shape: conditional
Havingrenders unconditionally. Conditional .Having(...) on a split GroupBy chain is not mask-gated — HAVING renders unconditionally #328 was retitledto that. Entity-terminal chains (no explicit Select) are silently not intercepted — CS9177 arity mismatch papered over by blanket NoWarn #329 was reclassified from CS9177 (warning) to CS9144 (error), making the
.Select(...)workarounds load-bearing rather than cosmetic.
<NoWarn>CS9177</NoWarn>was found vestigial (a full build with thesuppression overridden reported zero CS9177) and removed, rather than kept as planned. The "exact
expected set of CS9177" is empty, so the guard asserts zero CS9144/CS9177 across a shape matrix plus
proof an interceptor was actually emitted. A compilable Entity-terminal chains (no explicit Select) are silently not intercepted — CS9177 arity mismatch papered over by blanket NoWarn #329 pin was recovered after step 6 concluded
there was none.
assertions and by pg/my/ss sides that only assert
Has.Count. Sixteen of the twenty-one files in thefinal group needed nothing, and for informative reasons:
Composition/WindowFunctionwere alreadyorder-independent,
ConditionalMaskcarries an unconditionalOrderByin every chain, andOrderBy/DistinctOrderBypin a top-levelORDER BYthat a client-side sort would mask.curl+ REST rather thangh, which is not guaranteedon the self-hosted benchmark runner.
Gaps in original plan implemented
Found by the review pass and fixed here:
them" —
git diff --exit-codeon a directory nothing wrote is empty. The goldens are now deleted beforethe build, so the check also proves regeneration; untracked goldens are caught too, and
.gitattributeskeeps the comparison platform-independent.
title-only dedupe plus an advancing baseline meant one open issue silenced the channel permanently; and
a legitimate benchmark rename red-locked the pipeline with no override.
AssertNoStageRecomputedDifferentlypassed vacuously when no stage was tracked — in the flagshipassertion for the review's only High finding.
stayed green after the bug was fixed.
easy direction, and a false invalidation hides in the other one.
changes generator classification; they are now one shared helper.
Performance Considerations
DisplayClassEnricherbuilt a linear scan ofcompilation.SyntaxTreesper stale site, on a path thatfires for every site on a warm rebuild. Now a
FilePath → SyntaxTreedictionary built once.The concurrency suite costs ~35s (24 harnesses across 3 tests);
Workersis a single const if CI timeneeds trimming.
Security Considerations
issues: writeis required to file a regression issue, but the benchmark job also runs arbitrary projectand NuGet code on a persistent self-hosted runner. Issue filing therefore lives in a separate
ubuntu-latestjob that consumes only the perf-gate artifact; the benchmark job stayscontents: read.Values crossing the shell boundary in the new workflow logic go through
jq --arg/--argjson/--rawfile,which parse rather than interpolate; the only interpolated identifier is
${GITHUB_REPOSITORY}. Benchmarknames and the commit subject flow verbatim into an issue body, so an
@mentionin a commit subject wouldnotify — presentational only.
Breaking Changes
existing behaviour more robust.
Quarry.Tests.csprojdrops the blanket<NoWarn>CS9177</NoWarn>and gains<WarningsAsErrors>CS9177;CS9144</WarningsAsErrors>— an interceptor arity or signature mismatch nowbreaks that build instead of scrolling past.
TrackingNamesisinternal, notpublic; it was briefly added to the shippedQuarry.Generatorpackage's surface for a test-only concern, and reaches tests through the existing
InternalsVisibleTo..gitattributesis new — contributors on Windows withcore.autocrlf=falsemay see the manifestgoldens normalize on their next checkout.