Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The fixture ignores the configured external database connection string and starts a container without first checking the default local database, breaking external-database runs and forcing Docker on existing local setups.
2 open issues
What changed in this PR
Adds Testcontainers-based PostgreSQL setup to simplify running Npgsql tests with a local Docker engine, aiming to preserve existing database configurations.
Changes:
- Adds container fallback and SSL configuration to the NUnit test setup.
- Moves specification fixtures to asynchronous container initialization and cleanup.
- Adds centrally managed Testcontainers dependencies.
| File | Description |
|---|---|
| test/Npgsql.Tests/TestUtil.cs | Allows setup to replace the shared connection string. |
| test/Npgsql.Tests/Support/PostgreSqlContainerExtensions.cs | Generates SSL certificates and enables container SSL. |
| test/Npgsql.Tests/Support/AssemblySetUp.cs | Adds container fallback, image selection, and cleanup. |
| test/Npgsql.Tests/Npgsql.Tests.csproj | References PostgreSQL Testcontainers. |
| test/Npgsql.Specification.Tests/NpgsqlSelectValueFixture.cs | Moves table setup and cleanup into asynchronous lifecycle hooks. |
| test/Npgsql.Specification.Tests/NpgsqlDbFactoryFixture.cs | Introduces container-backed fixtures for specification tests. |
| test/Npgsql.Specification.Tests/NpgsqlConnectionTests.cs | Uses the connection-specific fixture. |
| test/Npgsql.Specification.Tests/NpgsqlCommandTests.cs | Uses the command-specific fixture. |
| test/Npgsql.Specification.Tests/Npgsql.Specification.Tests.csproj | References PostgreSQL and xUnit Testcontainers packages. |
| Directory.Packages.props | Defines shared Testcontainers package versions. |
Review effort: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The Docker fallback lacks required replication settings, uses fixed container names that collide across concurrent runs, and can override an explicitly configured database in specification tests.
3 open issues
2 resolved since last review
Review effort: Balanced
| catch when (Environment.GetEnvironmentVariable("NPGSQL_TEST_DB") == null) | ||
| { | ||
| // Connection to the default connection string failed, use Docker to run PostgreSQL | ||
| _postgreSqlContainer = new PostgreSqlBuilder(ContainerImage).WithName(ContainerName).Build(); |
|
|
||
| public override string ConnectionString => _connectionString; | ||
|
|
||
| protected override PostgreSqlBuilder Configure() => new PostgreSqlBuilder("postgres:18").WithName(ContainerName); |
Comment on lines
+41
to
+43
| using var rsa = RSA.Create(2048); | ||
| var request = new CertificateRequest(new X500DistinguishedName("CN=localhost"), rsa, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1); | ||
| using var certificate = request.CreateSelfSigned(notBefore: now, notAfter: now.AddDays(1)); |
| await container.CopyAsync(Encoding.Default.GetBytes(certPem), sslCertPath, uid, gid, certMode); | ||
| await container.CopyAsync(Encoding.Default.GetBytes(keyPem), sslKeyPath, uid, gid, keyMode); | ||
|
|
||
| await container.ExecScriptAsync(configureSsl); |
Without proper SSL configuration, all the SecurityTests would be skipped.
There was a problem hiding this comment.
🟡 Changes recommended
Teardown fails for external-database runs, missing host authentication breaks physical replication tests, and fixed container names cause collisions between concurrent test runs.
6 open issues
Review effort: Balanced
Comment on lines
+42
to
+53
| await base.InitializeAsync(); | ||
| _connectionString = base.ConnectionString; | ||
| } | ||
| } | ||
|
|
||
| protected override async Task DisposeAsyncCore() | ||
| { | ||
| if (_connectionString == base.ConnectionString) | ||
| { | ||
| await base.DisposeAsyncCore(); | ||
| } | ||
| } |
| await container.CopyAsync(Encoding.Default.GetBytes(certPem), sslCertPath, uid, gid, certMode); | ||
| await container.CopyAsync(Encoding.Default.GetBytes(keyPem), sslKeyPath, uid, gid, keyMode); | ||
|
|
||
| await container.ExecScriptAsync(configureSsl); |
This branch has not been deployed
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.
Note that Testcontainers will only be used if a connection to the default connection string (
Server=localhost;User ID=npgsql_tests;Password=npgsql_tests;Database=npgsql_tests) can't be established and theNPGSQL_TEST_DBenvironment variable is not defined. This way, existing tests setup continue to work as before but someone freshly onboarded can just have a local Docker engine running and the Npgsql tests will run (and pass) without any database setup effort.