Add default testcontainer support in the tests#3753
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes the test infrastructure by adding automatic testcontainer support for PostgreSQL when no connection string is explicitly configured. It removes the hard-coded config.json file and renames the DefaultConnection property to ConnectionString to better reflect its purpose.
Changes:
- Adds Testcontainers.PostgreSql package (v4.10.0) to automatically start a PostgreSQL container when no configuration is provided
- Removes config.json file and its copy directive from the project file
- Updates TestEnvironment to fall back to testcontainers when Test__Npgsql__DefaultConnection is not configured
- Renames
DefaultConnectionproperty toConnectionStringacross all test files
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/EFCore.PG.FunctionalTests/config.json | Removes hard-coded connection configuration file |
| test/EFCore.PG.FunctionalTests/TestUtilities/TestEnvironment.cs | Adds testcontainer fallback logic and renames property from DefaultConnection to ConnectionString |
| test/EFCore.PG.FunctionalTests/TestUtilities/TestNpgsqlRetryingExecutionStrategy.cs | Updates reference from DefaultConnection to ConnectionString |
| test/EFCore.PG.FunctionalTests/TestUtilities/NpgsqlTestStore.cs | Updates reference from DefaultConnection to ConnectionString |
| test/EFCore.PG.FunctionalTests/Query/NavigationTest.cs | Updates reference from DefaultConnection to ConnectionString |
| test/EFCore.PG.FunctionalTests/Migrations/MigrationsInfrastructureNpgsqlTest.cs | Updates reference from DefaultConnection to ConnectionString |
| test/EFCore.PG.FunctionalTests/EFCore.PG.FunctionalTests.csproj | Adds Testcontainers.PostgreSql package reference and removes config.json copy directive |
| Directory.Packages.props | Adds Testcontainers.PostgreSql version specification |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/EFCore.PG.FunctionalTests/TestUtilities/TestEnvironment.cs
Outdated
Show resolved
Hide resolved
test/EFCore.PG.FunctionalTests/TestUtilities/TestEnvironment.cs
Outdated
Show resolved
Hide resolved
When the environment variable isn't defined
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| if (Config["DefaultConnection"] is { } connectionString) | ||
| { | ||
| Console.WriteLine("Using connection string configured via Test:Npgsql:DefaultConnection: " + connectionString); |
There was a problem hiding this comment.
Console.WriteLine logs the full configured connection string, which may include credentials (e.g., passwords) and can leak secrets in CI logs or developer output. Consider avoiding logging the connection string entirely, or at least redact sensitive keys (Password/Username) before writing to output.
| Console.WriteLine("Using connection string configured via Test:Npgsql:DefaultConnection: " + connectionString); | |
| Console.WriteLine("Using connection string configured via Test:Npgsql:DefaultConnection."); |
| if (Config["DefaultConnection"] is { } connectionString) | ||
| { | ||
| Console.WriteLine("Using connection string configured via Test:Npgsql:DefaultConnection: " + connectionString); | ||
|
|
||
| private const string DefaultConnectionString = "Server=localhost;Username=npgsql_tests;Password=npgsql_tests;Port=5432"; | ||
| ConnectionString = connectionString; | ||
| } |
There was a problem hiding this comment.
The presence check Config["DefaultConnection"] is { } connectionString treats an empty/whitespace value as configured, which will set ConnectionString to an invalid value and skip the testcontainer fallback. Consider using !string.IsNullOrWhiteSpace(connectionString) (or trimming) to decide whether to start the container.
| Console.WriteLine("No connection string configured via Test:Npgsql:DefaultConnection, starting up testcontainer..."); | ||
|
|
||
| public static string DefaultConnection | ||
| => Config["DefaultConnection"] ?? DefaultConnectionString; | ||
| _postgreSqlContainer = new PostgreSqlBuilder("postgres:latest") | ||
| .WithCommand("-c", "max_connections=200") | ||
| .Build(); | ||
| _postgreSqlContainer.StartAsync().GetAwaiter().GetResult(); | ||
| ConnectionString = _postgreSqlContainer.GetConnectionString(); |
There was a problem hiding this comment.
The testcontainer fallback uses postgres:latest, but this test suite has PostGIS-dependent tests (RequiresPostgisAttribute checks for the postgis extension). Using a plain Postgres image means PostGIS tests will be skipped (or fail if NPGSQL_TEST_POSTGIS is set), reducing parity with CI. Consider switching to a PostGIS-enabled image (e.g., postgis/postgis:*) and/or avoiding the latest tag to keep the fallback reproducible.
When the environment variable isn't defined