perf(test): stop re-provisioning the SDK database on every test - #1702
Open
renemadsen wants to merge 3 commits into
Open
perf(test): stop re-provisioning the SDK database on every test#1702renemadsen wants to merge 3 commits into
renemadsen wants to merge 3 commits into
Conversation
TestBaseSetup's [SetUp] paid ~72s/test in fixture provisioning regardless of what the test needed, and CI ran redundant, unused MariaDB/RabbitMQ containers. Three fixes: - 420_SDK.sql: drop the frozen __EFMigrationsHistory DROP/CREATE/LOCK/ INSERT/UNLOCK block (mid-2024 snapshot) so EnsureCreated()/Migrate() own migration history instead of it being reset before every test. - TestBaseSetup.cs: make SDK database provisioning (EnsureCreated + SQL dump + Migrate, ~50-60s) lazy — only classes that call GetCore() pay for it, and only on first use, preserving the GetContext() -> StartSqlOnly() ordering. Also fixes a per-test leaked DbContext/MySQL connection by disposing it in OneTimeTearDown. - dotnet-core-pr.yml / dotnet-core-master.yml: remove the unused "Start MariaDB", "Start rabbitmq", and "Sleep 15" steps from the test-dotnet job only (TestBaseSetup uses its own ephemeral Testcontainers.MariaDb; nothing in the test project touches RabbitMQ). pn-playwright-test is untouched — it still needs its own mariadbtest/my-rabbit containers. Measured: BreakPolicyControllerTests.Create_WithNestedRules_ReturnsSuccess (a class that never calls GetCore()) dropped from a documented ~72s/test baseline to 42s reported / 44.9s total dotnet-test wall time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgEyDcnAEBcCF63RX2vm1k
…from per-test data reset Review round 1 caught two correctness regressions in the prior commit (3e8e46d): 1. Removing the __EFMigrationsHistory DROP/CREATE/LOCK/INSERT/UNLOCK block from 420_SDK.sql broke Migrate() for every GetCore()-calling test class. That block is the ONLY thing that populates __EFMigrationsHistory: EnsureCreated() never creates it, and ExecuteSqlRaw(dump) drops/recreates every real table at the frozen mid-2024 schema. Without the history block, Migrate() sees an empty history table, concludes zero migrations have been applied, and replays every migration since 2018 against tables the dump already created -> "table already exists", a hard failure. DO NOT remove this block again as a "dead weight" optimization -- it is load-bearing. The e2e fixture copy at eform-client/playwright/e2e/plugins/time-planning-pn/a/420_SDK.sql keeps the same block for the same reason. Restored the block verbatim (frozen at the same 20240619132520_AddPinCodeEmployeeNoToWorker migration point -- that's fine, EnsureSdkDbProvisionedAsync's first-time Migrate() brings the schema current from there). 2. Under 3e8e46d's lazy provisioning, SDK data was never reset between tests within the same fixture instance -- only Migrate() was skipped after the first test, but so was the data-resetting dump replay. Demonstrated concretely against MobileFlexRecomputeAndCascadeTests, which seeds Site/Worker/SiteWorker rows with fixed MicrotingUids in [SetUp]: with no per-test reset, its second test would insert duplicates (__EFMigrationsHistory.MicrotingUid has no unique constraint, PK only) -- a landmine other multi-test GetCore() classes were avoiding only by luck of disjoint ID ranges, not by guarantee. Split EnsureSdkDbProvisionedAsync so Migrate() (~44-48s, the expensive part) still runs exactly once per fixture, but every call after the first now replays the SQL dump (~7s) against the already-migrated schema to reset SDK *data* without re-running Migrate(). Per-test cost for GetCore() classes: ~7s dump replay + ~21s plugin migrate, versus the original ~72s/test and versus zero isolation in 3e8e46d. Verified by execution: a single test in MobileFlexRecomputeAndCascadeTests (which calls GetCore() in [SetUp]) now passes in 2m16s -- proof Migrate() against the restored history block no longer fails. See fix-round-1 section of stage0-report.md for the full verification writeup, including what was confirmed by execution versus by inspection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgEyDcnAEBcCF63RX2vm1k
There was a problem hiding this comment.
🟡 Changes recommended
GetCore() can now trigger multiple SDK DB re-seeds within a single test when called from both [SetUp] and the test body, which can wipe setup state mid-test and add avoidable runtime.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR reduces .NET test runtime by avoiding expensive SDK DB provisioning work on every test and by removing unused container startup steps from the test-dotnet CI workflow jobs.
Changes:
- Lazily provisions the SDK database on first
GetCore()use and replays the SQL dump on subsequent uses, plus disposes the previously leakedMicrotingDbContext. - Simplifies the
test-dotnetGitHub Actions jobs by removing MariaDB/RabbitMQ container startup and an unconditional sleep. - Keeps plugin DB setup per-test as before.
File summaries
| File | Description |
|---|---|
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/TestBaseSetup.cs | Makes SDK DB provisioning lazy/fixture-scoped and adds disposal of MicrotingDbContext. |
| .github/workflows/dotnet-core-pr.yml | Removes unused container startup/sleep steps from test-dotnet. |
| .github/workflows/dotnet-core-master.yml | Removes unused container startup/sleep steps from test-dotnet. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+130
to
+153
| /// per test. On every call after the first, the SDK *data* is reset by | ||
| /// replaying the SQL dump (~7s) without re-running Migrate(), so every | ||
| /// test that calls <see cref="GetCore"/> still gets an isolated SDK | ||
| /// database — classes that never call GetCore() pay neither cost. | ||
| /// </summary> | ||
| private async Task EnsureSdkDbProvisionedAsync() | ||
| { | ||
| if (_mariadbTestcontainer.State == TestcontainersStates.Undefined) | ||
| { | ||
| await _mariadbTestcontainer.StartAsync(); | ||
| } | ||
|
|
||
| if (MicrotingDbContext == null) | ||
| { | ||
| var dbContext = GetContext(_mariadbTestcontainer.GetConnectionString()); | ||
| dbContext.Database.SetCommandTimeout(300); | ||
| MicrotingDbContext = dbContext; | ||
| return; | ||
| } | ||
|
|
||
| var file = Path.Combine("SQL", "420_SDK.sql"); | ||
| var rawSql = await File.ReadAllTextAsync(file); | ||
| await MicrotingDbContext.Database.ExecuteSqlRawAsync(rawSql); | ||
| } |
Review round 2 (PR #1702 CI: 3 dotnet shards failed, e/g/h) caught a bug in 81f82c7's per-test SDK data reset: EnsureSdkDbProvisionedAsync replayed the SQL dump on every GetCore() call after the first, not once per test. A test that calls GetCore() more than once (e.g. to build a second Core after seeding SDK data via the first) had its own writes wiped out by its second call, before ever reading them back. Failure shape matched exactly: WorkingHoursExcelExportTagsColumnTests and sibling Excel-export tests build up SDK-backed export data across multiple GetCore() calls and assert on generated sheet content -- the export ran against a database that had just been reset out from under it, so sheets came back with only header rows. MobileFlexRecomputeAndCascadeTests passed throughout because both of its GetCore() calls happen in [SetUp] before any data it needs exists, so the bug was invisible to it -- not evidence the mechanism was safe. Fix: split provisioning from data reset cleanly. - EnsureSdkDbProvisionedAsync (called from GetCore()) now ONLY provisions once per fixture (EnsureCreated + dump + Migrate on the first call) and never resets data again -- repeated GetCore() calls within a test are now harmless, restoring the invariant the pre-Stage-0 code had for free. - New ResetSdkDbDataAsync replays the dump (~7s) without re-running Migrate() (~44-48s, still once-per-fixture). - [SetUp] now calls ResetSdkDbDataAsync once per test, but only when MicrotingDbContext != null -- i.e. only when an earlier test in this fixture already triggered provisioning. The first test's own EnsureSdkDbProvisionedAsync call already leaves SDK data freshly loaded from the dump, so [SetUp] skips a redundant reset before provisioning has happened at all. Verified by execution: WorkingHoursExcelExportTagsColumnTests .AllWorkersExport_TotalAndPerSiteSheets_TagsColumnAfterNameColumn (one of the tests CI reported failing on this exact bug) now passes in 2m16s, run alone, single method, foreground, with a hard timeout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgEyDcnAEBcCF63RX2vm1k
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.
The measured problem
TestBaseSetup's[SetUp]runs before every test method. Measured with a stopwatch across three local runs:MicrotingDbContext.Migrate()ExecuteSqlRaw(19 MB dump)TimePlanningPnDbContext.Migrate()EnsureDeleted/EnsureCreated/ seedConsequence:
SettingsServiceTeststakes 56 minutes for 20 tests; the full suite is ~326 minutes. Class cost tracks test count, not complexity — the signature of per-test fixture cost.What changes
SDK provisioning is now lazy and amortised. It happens on first
GetCore()rather than unconditionally in[SetUp]. Only 20 of 38 classes callGetCore()— the other 18 were paying ~50s per test for a database they never touch.Per-test isolation is preserved.
Migrate()runs once per fixture, but the SQL dump is replayed on every subsequent call (~7s) so each test still gets a clean SDK database. This matters: without it, classes with fixedMicrotingUids insert duplicateSite/Workerrows on their second test, since those columns have no unique constraint.A leaked
MicrotingDbContext(and its MySQL connection) per test is now disposed.Dead CI steps removed from
test-dotnetonly: it started amariadbtestMariaDB container, asome-rabbitRabbitMQ container and a hardcodedsleep 15, none of which the test project touches —TestBaseSetupuses its own Testcontainers instance on an ephemeral port.pn-playwright-testis untouched; it genuinely seeds those containers and points the app under test at them.What this does NOT do
It does not hit a 7-minute-per-shard target. Projected from the measured ~45s/test saving:
SettingsServiceTestsContentHandoverServiceTestsBreakPolicyServiceTestsThe saving applies to every test after the first in a class, so many-test classes gain most.
SettingsServiceTestsremains the suite's ceiling and needs separate investigation — the plugin-sideEnsureDeleted()+Migrate()(~21s/test) is still per-test, and its remaining per-test cost is larger than that alone explains.Review found two Criticals; both fixed and verified by execution
An earlier version of this change also deleted the
__EFMigrationsHistoryblock from420_SDK.sql. That block must stay. It is the only thing populating that table; without itMigrate()sees an empty history and replays every migration against tables the dump already created — a hard failure, not slowness. The e2e fixture copy ateform-client/playwright/e2e/.../420_SDK.sqlkeeps it for the same reason. Restored.The isolation regression above was also found in review, demonstrated against
MobileFlexRecomputeAndCascadeTests, and fixed rather than documented away.Both verified by running
MobileFlexRecomputeAndCascadeTests(aGetCore()-calling class, two tests): 2/2 passed. First test 2m11s (full provisioning), second 1m38s — a ~38s reduction consistent with skippingMigrate(), and no duplicate-row failure.🤖 Generated with Claude Code
https://claude.ai/code/session_01TgEyDcnAEBcCF63RX2vm1k