fix(tests): clean documents when IntegrationContext.StoreOptions builds a store (#398) - #401
Merged
Merged
Conversation
…ds a store (#398) IntegrationContext.StoreOptions applied schema changes but never removed data, so every test routed through it inherited whatever earlier runs left in its schema. The suite isolates by DatabaseSchemaName inside one shared `master` database rather than by database, so that residue survives across runs indefinitely and any test asserting on an absolute count drifts upward — e.g. single_tenant_has_no_tenant_id_column_tests.full_lifecycle_works_without_tenant_id_column saw byLinq.Count grow 1 -> 2 -> 3 across successive local runs. CI never sees it because it provisions a fresh SQL Server per run, which made this present locally as a flaky assertion in an area the developer did not touch. StoreOptions now empties the configured schema's document tables after applying the schema, via the store's own Advanced.Clean.DeleteAllDocumentsAsync(). Deleting rows rather than dropping the schema (Marten's approach) is deliberate: a Polecat StoreOptions call that does not set DatabaseSchemaName lands on the shared `dbo` schema, and dropping that would take the fixture's own tables with it. A `cleanAll` opt-out is provided for the one test that reconfigures a store mid-test and needs the previously written row to survive — document_range_partitioning_tests.additive_boundary_rolls_a_new_partition_forward_without_losing_data, whose whole point is that an in-place SPLIT RANGE preserves data. A scan for StoreOptions calls that follow a save within the same test method found no others. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G8tN8ApXiKhyVzia4iwmof
Follow-up to the #398 fix. The clean was emptying every pc_doc_* table in the configured schema, which for the 39 of 225 call sites that do not set their own DatabaseSchemaName means `dbo` — the collection-wide DefaultStoreFixture's schema, shared with every other class in the "integration" collection. Wiping it trades one cross-run leak for a cross-class one, and it is also by far the most expensive case, since `dbo` accumulates a pc_doc_* table for nearly every document type in the suite. Skipping the default schema keeps the fix for what the issue actually reported (both the originally-failing class and every other count-asserting caller set their own schema) while removing 39 whole-schema sweeps from each run. Worth doing on its own merits, and it also takes load off the slower Azure SQL Edge CI job, where this PR's first run hit connection- pool exhaustion 1449 tests in — a pre-existing marginal condition (a clean re-run of the same commit passed), but not one worth pushing closer to the edge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G8tN8ApXiKhyVzia4iwmof
Merged
jeremydmiller
added a commit
that referenced
this pull request
Aug 3, 2026
Minor rather than patch: 5.9.1's line added public surface (IEventBinarySerializer, EventStoreOptions.AddEventType/AddEventTypes) and moved the whole JasperFx/Weasel matrix forward. Since 5.9.1: - feat: pluggable binary event serialization via IEventBinarySerializer (#388/#402) - feat: EventStoreOptions.AddEventType / AddEventTypes (#395/#396) - fix: escape interpolated identifiers and literals in constructed SQL (#390/#403) - fix: throw a lone DcbConcurrencyException unwrapped from SaveChangesAsync (#394/#397) - deps: JasperFx 2.37.2 -> 2.38.0, Weasel 9.23.0 -> 9.23.2 (#405, #407) - Polecat's ProjectionScenario is now a thin subclass of the lifted JasperFx.Events.TestSupport harness rather than a seven-file copy of Marten's (#404/#408, jasperfx#616) -- a behavior change for anyone already using it, see the release notes - test infrastructure: compliance waves 1-3 (#393, #400, #407), parallel-safe test suite (#389), IntegrationContext.StoreOptions document cleaning (#398/#401)
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.
Closes #398.
The problem
IntegrationContext.StoreOptions(...)applied schema changes but never removed data, so every test routed through it inherited whatever earlier runs left in its schema. The suite isolates byDatabaseSchemaNameinside one sharedmasterdatabase rather than by database, so that residue survives across runs indefinitely and any test asserting on an absolute count drifts upward.The reported symptom —
single_tenant_has_no_tenant_id_column_tests.full_lifecycle_works_without_tenant_id_columnseeingbyLinq.Countgrow 1 → 2 → 3 across successive local runs — is local-only, because CI provisions a fresh SQL Server per run. That is what made it confusing: it presents as a flaky assertion in an area the developer did not touch.The fix
StoreOptionsnow empties the configured schema's document tables after applying the schema, through the store's ownAdvanced.Clean.DeleteAllDocumentsAsync().Deleting rows rather than dropping the schema (Marten's
OneOffConfigurationsContextdoesdrop schema … cascade) is deliberate: a PolecatStoreOptionscall that does not setDatabaseSchemaNamelands on the shareddboschema, and dropping that would take the fixture's own tables with it.The audit the issue asked for
With
cleanAlldefaulting to true the count question is moot — every caller gets a clean slate. The audit that actually matters is the inverse: which tests depend on pre-existing data surviving into the store. A scan of all 222StoreOptionscall sites for a data write earlier in the same test method found exactly two, and a full-suite run confirmed the same two and no others:document_range_partitioning_tests.additive_boundary_rolls_a_new_partition_forward_without_losing_data— reconfigures mid-test; the whole point is that an in-placeSPLIT RANGEpreserves the row written under the previous configuration.strong_typed_id_column_type_tests.migrates_legacy_varchar_id_column_in_place_preserving_data— seeds a legacyvarchar(250)id row through a raw connection beforeStoreOptions, then asserts the in-place id conversion preserved it.Both now pass
cleanAll: false, matching Marten's opt-out parameter of the same name.Verification
Full
Polecat.Testssuite locally against the dockerized SQL Server 2025, net10.0:strong_typed_idcase above), 1634 passed, 3 skipped — i.e. the change caused exactly one regression, which is now fixed🤖 Generated with Claude Code
https://claude.ai/code/session_01G8tN8ApXiKhyVzia4iwmof