feat: Share common interface (IDatabaseContainer) for ADO.NET compatible containers #920
Conversation
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Thank you. That is something I have been thinking about adding again as well. I would like to propose that we only take into account modules that are compatible with ADO.NET (similar to TC for Java, which consideres support of JDBC). Perhaps it would be a good idea to rename the interface to something like IAdoDotNetDatabaseContainer. WDYT?
This will also be required to implement a new wait strategy for database where an ADO.NET provider is available.
What do you mean by that? Why would that be necessary?
I'd rather have all containers that provide a connection string to implement the new public interface IAdoDotNetDatabaseContainer
{
string GetConnectionString();
DbProviderFactory DbProviderFactory { get; }
}And I'd rather let the choice of the ADO.NET provider to the
Because the connection string must come from the container itself. Here's my prototype of a new wait strategy that waits until the database is actually available. We see that a new interface is required to implement this strategy if we don't want to use reflection on the namespace DotNet.Testcontainers.Configurations
{
using System;
using System.Data.Common;
using System.Diagnostics;
using System.Threading.Tasks;
using DotNet.Testcontainers.Containers;
internal class UntilDatabaseIsAvailable : IWaitUntil
{
private static readonly TimeSpan DefaultFrequency = TimeSpan.FromSeconds(1);
private static readonly TimeSpan DefaultTimeout = TimeSpan.FromMinutes(1);
private readonly DbProviderFactory _dbProviderFactory;
private readonly TimeSpan _frequency;
private readonly TimeSpan _timeout;
public UntilDatabaseIsAvailable(DbProviderFactory dbProviderFactory, TimeSpan frequency, TimeSpan timeout)
{
_dbProviderFactory = dbProviderFactory;
_frequency = frequency == default ? DefaultFrequency : frequency;
_timeout = timeout == default ? DefaultTimeout : timeout;
}
public async Task<bool> UntilAsync(IContainer container)
{
if (!(container is IDatabaseContainer dbContainer))
{
throw new InvalidOperationException($"{container.GetType().FullName} must implement the {nameof(IDatabaseContainer)} interface in order to wait until the database is available.");
}
var stopwatch = Stopwatch.StartNew();
var connectionString = dbContainer.GetConnectionString();
while (!await IsAvailableAsync(connectionString, stopwatch))
{
await Task.Delay(_frequency);
}
return true;
}
private async Task<bool> IsAvailableAsync(string connectionString, Stopwatch stopwatch)
{
using (var connection = _dbProviderFactory.CreateConnection() ?? throw new InvalidOperationException($"{_dbProviderFactory.GetType().FullName}.CreateConnection() returned null."))
{
connection.ConnectionString = connectionString;
try
{
await connection.OpenAsync();
return true;
}
catch (DbException exception)
{
if (stopwatch.Elapsed > _timeout)
{
throw new TimeoutException($"The database was not available at \"{connectionString}\" after waiting for {_timeout.TotalSeconds:F0} seconds.", exception);
}
return false;
}
}
}
}
}This is how I envisioned it but I'm totally open to alternatives. |
0xced
force-pushed
the
IDatabaseContainer
branch
from
0658103
to
333a5d0
Compare
There was a problem hiding this comment.
I'd rather have all containers that provide a connection string to implement the new
IDatabaseContainerinterface.
I understand your perspective, but implementing the IDatabaseContainer interface for modules that are not associated with a database like LocalStack or WebDriver sounds not right.
Coupling the connection string to ADO.NET providers would not really make sense without an additional DbProviderFactory property and thus taking a dependency on specific providers.
My intention was not to connect them specifically to ADO.NET providers. I simply wanted to include the interface in those modules because they have something in common. The other modules do not share this similarity.
And I'd rather let the choice of the ADO.NET provider to the
Testcontainersconsumer.
OC, that is why (as you described below), we do not include such providers, client libraries, etc. in our dependencies.
Because the connection string must come from the container itself. Here's my prototype of a new wait strategy that waits until the database is actually available.
I am still not able to understand it completely. What is the issue with the current wait strategies? Based on our experience, relying solely on the first database connection often leads to unreliable test results. We have frequently observed situations where the databases allowed connections but were not yet ready to process any data, causing the tests to fail. The current implementation of the wait strategies is shared across TC language implementations and is considered stable.
I agree. Happy to remove the
Using the following configuration hangs the PostgreSQL container forever (the WithVolumeMount("Testcontainers.PostgreSql.Data", "/var/lib/postgresql/data")But my idea was not to replace the current strategies, only to add another possible strategy when a
My experience differs. Successfully opening a connection to a database with the most common ADO.NET drivers has been my strategy of choice and worked well for me. This is what I have implemented in my DockerRunner library but I don't plan to maintain it, hence my interest in |
That would be greate, thanks.
Interesting, I haven't tried it that way. Odd that the log message differs. Can you share the container's output?
I am just worried that it won't indicate readiness reliable. We had a couple of issues in the past (but we used the container's CLI tools to test the database connection). As long as it is reliable, we can ship it with TC. Does it make sense to split it in separate PRs? |
0xced
force-pushed
the
IDatabaseContainer
branch
from
333a5d0
to
459250d
Compare
I have removed And since I rebased on the develop branch, I also added the
I'll open a separate issue about this because there's already enough to discuss. 😉
Yes, it totally makes sense in two separate pull requests. |
0xced
force-pushed
the
IDatabaseContainer
branch
from
459250d
to
61c92a9
Compare
|
I just rebased on the I'll be able to open a new pull request to discuss a potential new wait strategy once this is merged. |
Sorry for my late response. I will try to take a look at both PRs this week. |
There was a problem hiding this comment.
Overall, I am fine with the PR, thanks for your patience once again. I still think that it makes more semantic sense to implement the interface only for containers compatible with IDbConnection (Maybe it makes sense to add a common interface to the Testcontainers modules that offers access to the module. However, aligning this interface with module-specific idiomatic language is challenging, as they refer to different names: connection string, endpoint, base address, etc.).
Especially where container instances are shared, as demonstrated in the example above. These instances must have some similarities, and I do not understand the advantages of using the interface for a shared PostgreSQL and Kafka instance. I am curious to hear your thoughts on this matter: @cristianrgreco, @eddumelendez, @kiview, @mdelapenya
Actually I agree. I have pushed 22f383c which does exactly this. I have improved the test to makes sure that It's a good thing I actually wrote the test like this because I would have missed both ClickHouse and SqlEdge. Also, the day when a Firebird test container will be added, referencing the FirebirdSql.Data.FirebirdClient package in the test project will automatically test that the container must implement Here's what the test results look like. 
|
This is required in order to implement a generic database fixture that could look like this.
```csharp
using System;
using System.Data.Common;
using System.Threading.Tasks;
using DotNet.Testcontainers.Builders;
using DotNet.Testcontainers.Containers;
using Testcontainers.MsSql;
using Testcontainers.MySql;
using Testcontainers.PostgreSql;
using Xunit;
namespace SampleCode;
public abstract class DatabaseFixture<TBuilder, TContainer> : IAsyncLifetime
where TBuilder : IContainerBuilder<TBuilder, TContainer>, new()
where TContainer : IContainer, IDatabaseContainer
{
private string _connectionString;
private TContainer _container;
protected abstract DbProviderFactory ProviderFactory { get; }
public string ConnectionString => _connectionString ?? throw new InvalidOperationException($"{nameof(IAsyncLifetime.InitializeAsync)} must be called before accessing the connection string");
async Task IAsyncLifetime.InitializeAsync()
{
_container = new TBuilder().Build();
await _container.StartAsync();
_connectionString = _container.GetConnectionString();
using var connection = ProviderFactory.CreateConnection() ?? throw new InvalidOperationException($"ProviderFactory.CreateConnection() returned null ({ProviderFactory})");
connection.ConnectionString = _connectionString;
await connection.OpenAsync();
await connection.CloseAsync();
}
async Task IAsyncLifetime.DisposeAsync()
{
if (_container != null)
{
await _container.StopAsync();
}
}
}
public class MsSqlDbFixture : DatabaseFixture<MsSqlBuilder, MsSqlContainer>
{
protected override DbProviderFactory ProviderFactory => Microsoft.Data.SqlClient.SqlClientFactory.Instance;
}
public class MySqlDbFixture : DatabaseFixture<MySqlBuilder, MySqlContainer>
{
protected override DbProviderFactory ProviderFactory => MySqlConnector.MySqlConnectorFactory.Instance;
}
public class PostgreSqlDbFixture : DatabaseFixture<PostgreSqlBuilder, PostgreSqlContainer>
{
protected override DbProviderFactory ProviderFactory => Npgsql.NpgsqlFactory.Instance;
}
public class PostgreSqlTest : IClassFixture<PostgreSqlDbFixture>
{
private readonly PostgreSqlDbFixture _dbFixture;
public PostgreSqlTest(PostgreSqlDbFixture dbFixture) => _dbFixture = dbFixture;
[Fact]
public async Task TestOnPostgreSql()
{
await using var connection = new Npgsql.NpgsqlConnection(_dbFixture.ConnectionString);
await using var command = connection.CreateCommand();
// ...
}
}
```
This will also be required to implement a new wait strategy for database where an ADO.NET provider is available.
…iner They aren't really databases.
0xced
force-pushed
the
IDatabaseContainer
branch
from
22f383c
to
f33d7e7
Compare
It is a smart idea 💡, although it is a bit fuzzy. It works as long as modules do not contain container implementations (n), where one container supports a .NET data provider, and one does not. If that is the case sometime, we will figure something out. For now, it looks good. Thanks. I will merge it later 🎉. |
HofmeisterAn
changed the title
|
Awesome, thanks for merging! Also thanks for tinkering the little details such as |
What does this PR do?
This PR introduces a new
IDatabaseContainerinterface with a single method:string GetConnectionString();All containers that expose a
GetConnectionString()method have been updated to also implement the newIDatabaseContainerinterface.Why is it important?
This is required in order to implement a generic database fixture that could look like this. The key is the
TContainer : IContainer, IDatabaseContainerconstraint.This will also be required to implement a new wait strategy for database where an ADO.NET provider is available.
Note that this issue came up when I tried to use
Testcontainersfor improving Dapper tests. Currently some tests are skipped if a database is not available. UsingTestcontainerswould enable running more tests when Docker is installed.Related issues
N/A
How to test this PR
A new
Testcontainers.Databases.Testsproject has been added to make sure that all container types that have aGetConnectionStringmethod implement this newIDatabaseContainerinterface. There are currently 21 containers that implement the newIDatabaseContainerinterface.Note that thanks to the ProjectReference globbing any new container added will be automatically tested and a test will fail if a
GetConnectionStringmethod exists but theIDatabaseContainerinterface is not declared.