Skip to content

Add integration tests, starting with a PostgreSQL testcontainer for the persistence layer #905

Description

@sven-n

Is your feature request related to a problem? Please describe.

Everything that only exists inside a real PostgreSQL is currently verified by a human starting the server and watching it come up: the 108 migrations, the role and GRANT generation in MyNpgsqlMigrationsSqlGenerator, the hand-written SQL in JsonQueryBuilder / GameConfigurationJsonQueryBuilder, the caching repository stack, and the 120 configuration update plug-ins.

State of testing today:

Observation Where
Ten test projects, all pure unit tests tests/
The only EF-against-a-real-database test is disabled tests/MUnique.OpenMU.Persistence.Initialization.Tests/TestInitializationWithEfCore.cs:30 ([Ignore])
The JSON query builder test builds the SQL but never executes it tests/MUnique.OpenMU.Persistence.Initialization.Tests/JsonQueryBuilderTests.cs:36
The configuration change test only inspects EF model metadata tests/MUnique.OpenMU.Persistence.Initialization.Tests/ConfigurationChangePublishingTests.cs:29
CI runs no tests at all — only dotnet publish of Startup .github/workflows/dotnetcore.yml:17

The last row matters most: until CI executes the suite that already exists, nothing added later can prevent a regression.

Describe the solution you'd like

Four tiers, of which the first two are worth doing now:

Tier Covers Docker Budget
0 The existing unit tests, finally executed in CI no ~1 min
1 Persistence against a real PostgreSQL (testcontainer) yes ~3–6 min
2 Login/connect/guild/friend/game server wired in-process, in-memory persistence, scripted network client no ~1–2 min
3 The tier 2 harness on tier 1 persistence — true end-to-end yes ~2 min
4 deploy/all-in-one compose smoke test, nightly yes ~5 min

The constraint that shapes the harness

ConnectionConfigurator is static and single-shot — a second Initialize throws — and every DbContext self-initializes it with the file provider on first use (EntityDataContext.cs:47, EntityDataContextFactory.cs:19, AdminAuth/AdminPanelContextFactory.cs:19, MyNpgsqlMigrationsSqlGenerator.cs:66). Three consequences:

  1. The container's connection settings must be installed before the first DbContext is constructed in the process — in NUnit, an assembly-level [SetUpFixture] outside any namespace.
  2. One connection-string set per test assembly, therefore one container per assembly.
  3. A container assembly cannot also host tests that use ConfigFileDatabaseConnectionStringProvider — which the existing persistence tests do. So this needs a new test project, not an extension of the existing one.

Connection settings for tests

A TestDatabaseConnectionSettingProvider loads the shipped ConnectionSettings.xml as its template and rewrites only host, port and database via NpgsqlConnectionStringBuilder. Keeping the production role names and passwords (account, config, guild, friend) means the migration generator's CREATE ROLE / GRANT statements run exactly as in production, which is what makes privilege tests possible at all. Since GetConnectionSetting is consulted on every OnConfiguring, the provider can serve a mutable database name, so switching databases between fixtures is a field assignment.

Container and database lifecycle

Seeding is the expensive part (CreateInitialDataAsync(1, true) builds the whole game configuration), so pay for it once:

once per assembly
  start postgres:<pinned>-alpine   -c fsync=off -c synchronous_commit=off
                                   -c full_page_writes=off -c max_connections=200
  migrate EntityDataContext + AdminPanelContext
  run data initialization          ->  database "openmu_template"

per fixture (writing tests only)
  NpgsqlConnection.ClearAllPools()
  CREATE DATABASE "test_n" TEMPLATE "openmu_template"    (a file copy)
  point the provider at test_n

Read-only fixtures share one database and skip the copy. Four traps worth writing into the harness up front:

  • Pooling — CREATE DATABASE … TEMPLATE and DROP DATABASE fail while any connection is open; clear the pools first.
  • Roles are cluster-wide, not per database — CREATE ROLE is guarded by an existence check, but dropping a schema emits DROP ROLE IF EXISTS, so never run two migration runs concurrently in one container.
  • ReCreateDatabaseAsync(true) issues EnsureDeletedAsync; on a freshly created database pass false and let migrations build the schema.
  • Parallelism — start the integration assembly NonParallelizable; revisit once the template path is proven.

Required changes in src/ (both small)

  1. ConnectionConfigurator: an Initialize(provider, overwrite: true) overload, or an internal Reset() plus InternalsVisibleTo (the repo already does this for other test assemblies). The file's own TODO: Make class non-static is the proper long-term fix.
  2. ConfigFileDatabaseConnectionStringProvider: support DB_PORT (and optionally DB_NAME) next to the existing DB_HOST, DB_ADMIN_USER, DB_ADMIN_PW. Useful on its own — a non-default port is not configurable today.

Tier 1 test catalogue, by value

  1. Schema health — all migrations apply to an empty database and leave nothing pending; roles exist with the expected grants; re-migrating is a no-op. Plus a model drift check comparing the snapshot against the current model with IMigrationsModelDiffer: PendingModelChangesWarning is currently suppressed (ConnectionConfigurator.cs:114), so "changed the model, forgot the migration" is invisible today.
  2. Data initialization round-trip for Version075, Version095d, VersionSeasonSix — the [Ignore]d test revived: seed, reload through GameConfigurationJsonObjectLoader, compare against the in-memory result (collection counts plus spot checks on maps, items, skills, drop groups); the documented test accounts must authenticate.
  3. The JSON loading paths, actually executed — JsonQueryBuilder and AccountRepository.LoadAccountByLoginNameByJsonQueryAsync (AccountRepository.cs:158).
  4. Role privileges — the config and account roles may not write configuration, the trade context is limited to items; assert PostgresException with SqlState 42501.
  5. Player data round-trip — account and character with inventory, vault and item options: save, reload in a fresh context, compare structurally; then letters, guild membership and friends.
  6. Concurrency — the real-database counterpart to the in-memory PersistenceLockTest.
  7. Configuration update plug-ins — apply all 120 IConfigurationUpdatePlugIns in version order to a freshly seeded database: each applies without error, is idempotent when applied twice, and the configuration still loads afterwards. Given how often updates are added, probably the strongest regression net here.
  8. Configuration change publishing — change an entity through a context, assert ConfigurationChangeListener publishes it and the cache-aware repository provider serves the updated instance.
  9. Admin panel authentication against the real admin schema — AdminUserRepository, bootstrap admin, BCrypt hashing, lockout.

Tier 2 and above

Tier 2 starts the servers in-process on ephemeral ports with in-memory persistence and drives them with a scripted client built from MUnique.OpenMU.Network (a Connection with the client-side SimpleModulus/XOR32 keys and the generated packet structs): connect handshake and server list, login with wrong and right credentials, character list/creation/deletion, enter world, walk, chat and whisper, logout and reconnect, the per-IP connection limit, disconnect cleanup. ChatServer.Tests/ChatClientTests.cs already shows the cheaper DuplexPipe variant; both are useful — pipes for protocol assertions, real loopback sockets for at least one smoke test of the listener and encryption wiring.

Tier 3 is that harness on the tier 1 persistence: play a short scripted session, then assert the persisted result (position, experience, inventory) — the test that catches "works in-memory, breaks on PostgreSQL".

CI

Keep the existing build job, add unit-tests (--filter TestCategory!=Integration) and integration-tests (--filter TestCategory=Integration), both on ubuntu-latest where Docker is available. Things that will otherwise bite:

  • Do not build the whole solution on Linux — ClientLauncher and Network.Analyzer.WinForms target net10.0-windows. Use a solution filter listing the test projects and their dependencies, or invoke dotnet test per project.
  • Do not pass -p:ci=true to the test jobs — the packet structure tests are generated in a PreBuild target that is skipped when it is set (tests/MUnique.OpenMU.Network.Packets.Tests/…csproj:43), so they would silently vanish.
  • Skip policy — missing Docker ignores the fixture locally but fails when the CI variable is set; a silently skipped suite in CI is worse than no suite.
  • Cache NuGet, pre-pull the image, and consider adding a pull_request trigger (the workflow is on: [push] only today).

Locally this needs Docker Desktop, Podman with DOCKER_HOST, or Rancher Desktop; --filter TestCategory!=Integration stays fast and container-free for everyone else, and Testcontainers reuse behind an environment switch keeps the inner loop short.

Suggested rollout

Phase Content Size
0 CI runs the existing unit tests (solution filter, no ci=true); no new tests small
1 Spike: new project, container, provider, template database, one migrate-and-seed test, measured timings medium
2 Schema health, data initialization round-trip, JSON loading (1–3) medium
3 Repositories, privileges, concurrency, admin auth (4–6, 9) medium
4 Configuration update plug-ins and change publishing (7–8) medium
5 Integration job in CI, contributor documentation small
6 Tier 2 server harness and the first flows large
7 Tier 3 end-to-end, optionally tier 4 nightly medium

Phases 0–2 already deliver most of the value; the rest can be picked up piece by piece.

Describe alternatives you've considered

  • A shared, externally provisioned PostgreSQL (docker-compose or a CI service container) instead of Testcontainers: fewer moving parts in the test code, but no per-test database isolation, no automatic lifecycle, and contributors have to remember to start it.
  • Only extending ConfigFileDatabaseConnectionStringProvider with DB_PORT/DB_NAME and pointing it at a container started outside the test process: less new code, but the settings are read once, so per-test databases are impossible. Worth doing anyway as a production improvement, just not sufficient as the harness.
  • Keeping everything on the in-memory provider: fast, but by construction it cannot cover migrations, the generated role SQL, the raw JSON queries or privilege enforcement — exactly the code that this proposal targets.
  • Transaction rollback per test instead of template databases: does not work here, because a single scenario spans several contexts on separate connections (account, config, guild, friend, trade).

Additional context

Open questions before phase 1:

  • Which PostgreSQL version should be the supported baseline? One pinned version is cheap, a matrix over two majors doubles the job time — and the deploy compose files use the unpinned postgres tag today (deploy/all-in-one/docker-compose.yml:42).
  • Should the integration job run on every push, or only on pull requests?
  • If Season 6 seeding turns out to be slow, is it acceptable to run the repository-level fixtures on Version075 data and keep the full seed for the initialization fixture only?

I'm happy to implement this phase by phase if the direction looks right.

No activity

Activity on this issue will appear here.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions