Skip to content

Test suite guardrails: hollow incremental-caching tests, no perf regression gate, concurrency insurance, row-order flakes, manifest goldens, streaming/cancellation #314

Description

@DJGosnell

Combined finding from the 2026-07-07 multi-agent deep review (tests perspective, grade B+). The suite's strengths are real (real 4-dialect Testcontainers execution, exact per-dialect SQL assertions, live generator as analyzer). These items are the verified blind spots — concentrated exactly where the project is most fragile.

1 (high, CONFIRMED — fixture executed during verification): IncrementalCachingTests never exercises the caching machinery on valid chains

Three independent hollow axes (src/Quarry.Tests/IncrementalCachingTests.cs):

  1. All query sources use anonymous-type projections (lines 47, 62, 170, 240), which ChainAnalyzer.cs:1269-1274 disqualifies into RuntimeBuild → QRY032. Verified by running the generator on the fixture sources: both chains produce QRY032 and the emitted interceptor files are empty shells (no SQL, no carrier). The cache assertions only ever cover the error path.
  2. PerFileOutput_UnchangedCompilation_AllOutputsCached re-runs the driver with the same CSharpCompilation instance, so every input is reference-equal and Roslyn reports Cached without invoking a single model .Equals — a broken IEquatable implementation is undetectable.
  3. PerFileOutput_ModifyOneFile_OtherFileCached asserts only Does.Contain(Cached) across ALL outputs (one cached step anywhere passes); PerFileOutput_ModifyQuery_RegeneratesAffectedFile captures initialInterceptorSources and never compares it.

The generator's own llm.md records that EntityRegistry equality omitting _allContexts was a shipped latent caching bug — and EntityRegistryTests.cs:68-76 still tests only positive equality. AssembledPlan/CarrierPlan/FileInterceptorGroup equality has no negative tests anywhere.

Work: rewrite the fixture with valid tuple/DTO projections and assert no QRY014/QRY032; re-parse identical source into fresh trees for the unchanged-run test; add WithTrackingName to pipeline stages and assert per-stage Cached/Unchanged reasons; assert the unmodified file's specific output step is Cached and the modified file's is Modified; add inequality tests for EntityRegistry and the other pipeline models. Also cover: a schema-only edit regenerating entities, and the cross-partial ordinal-shift scenario (see display-class issue).

2 (high, CONFIRMED): No automated performance regression gate

  • Benchmarks run only post-merge (benchmark.yml:3-8, workflow_run on CI success, master only) and publish to the gh-pages dashboard with zero threshold/alert/fail logic (publish step :132-248 only jq-appends; repo-wide grep finds no gate; the dashboard 'threshold' hits are unit-formatting tables). PRs never run benchmarks. BytesAllocatedPerOperation is recorded but never compared. A 10× hot-path regression merges green; detection depends on a human noticing a dashboard bend after the commit lands. README.md:72 claims "faster than Dapper" — performance is the stated identity.
  • Work: threshold check in the benchmark job (compare mean/allocations against the previous runs.json entry; fail or open an issue beyond e.g. 15% mean / any allocation increase on Quarry_* methods). Consider a fast PR-time smoke: hard allocation assertions via GC.GetAllocatedBytesForCurrentThread around the hot fetch paths in a plain test (deterministic, CI-friendly).
  • Related CI hole (from the benchmarks review): the merge step filters select(.Statistics != null) (benchmark.yml:93), silently dropping failed benchmarks from the dashboard — a broken benchmark vanishes instead of failing the run (already happened: AggregateSumBenchmarks Raw_Sum = NA in committed artifacts). Fail the workflow when any expected series is missing.

3 (medium, verifier-downgraded from high — insurance, not live defect): Zero concurrency testing

  • The only Interlocked/WhenAll/Parallel hit across all three test projects is a test logger's counter; no test ever executes two Quarry operations concurrently; NUnit runs single-threaded. Verification confirmed current shared state is safe by construction (eager static readonly init, ConcurrentDictionary, per-call locals, documented-benign _sqlCache race) — so this is regression insurance for future refactors of generated statics.
  • Work: small concurrency suite — N parallel harnesses running mixed SELECT/UPDATE/Patch via Task.WhenAll; parallel first-touch of interceptors sharing one carrier class (static-init race); parallel contexts on separate connections (the documented-supported scenario).

4 (medium): 526 positional row-order assertions on PG/MySQL/SQL Server without ORDER BY

  • RowOrderExtensions.cs:7-12 documents this exact flake mode and provides SortedByAsync, but it is used at only 31 call sites while pgResults[0]|myResults[0]|ssResults[0] positional indexing appears 526 times across SqlOutput/. One planner change converts hundreds of tests into intermittent failures at once.
  • Work: mechanical sweep to SortedByAsync (or add ORDER BY to the queries where SQL assertions permit).

5 (medium): Known generator bugs are routed around instead of pinned; blanket CS9177 suppression

  • CrossDialectConditionalMaskTests.cs:386-390: cross-context misattribution (chain binds to CteDb instead of TestDbContext — wrong-dialect SQL) noted as "Filed as follow-up" with no pinning test. PostgresIntegrationTests.cs:45-48: entity-terminal path on PG skipped due to an interceptor signature mismatch "tracked separately". Quarry.Tests.csproj:13-14: project-wide NoWarn CS9177 means some call sites are silently NOT intercepted and new arity mismatches are invisible — risky in a carrier-only architecture where an unintercepted call is a correctness hole.
  • Work: pin each known bug with an explicitly-failing/[Ignore]d test that signals when fixed; replace the blanket CS9177 suppression with targeted suppressions plus a test asserting the expected set of unintercepted sites.

6 (medium): SQL-manifest goldens have no CI enforcement

  • llm-testing.md calls ManifestOutput/quarry-manifest.{dialect}.md "checked-in goldens", but no test reads them and ci.yml has no git diff --exit-code step — generator SQL drift stays green in CI.
  • Work: CI step regenerating manifests and failing on diff (or a test comparing them).

7 (medium): Streaming and cancellation nearly untested

  • CrossDialectStreamingTests has 3 tests, all enumerating to completion; no early-break disposal test (leaked reader poisons the shared connection on MySQL/SQL Server); CancellationToken appears in one file. Each generator-emitted terminal shape needs behavioral tests.
  • Work: early-break + reader/command disposal tests; mid-stream cancellation tests; subsequent-command-on-same-connection assertions.

Low (bundled)

  • Suite runs fully sequentially — documented parallelizability never enabled (618 real-DB harness creations).
  • Display-class prediction unit tests validate the predictor against itself and only one Roslyn version — add a compile-and-execute canary (see display-class issue) and consider a multi-Roslyn CI leg.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions