testcontainers / testcontainers-dotnet

A library to support tests with throwaway instances of Docker containers for all compatible .NET Standard versions.
https://dotnet.testcontainers.org
MIT License
3.67k stars 254 forks source link

Make ESDB container implement IDatabaseContainer #1053

Closed alexeyzimarev closed 7 months ago

alexeyzimarev commented 8 months ago

What does this PR do?

EvebtStoreDbContainer class implements the GetConnectionString method so it can inherit from IDatabaseContainer.

Why is it important?

When creating an abstraction for tests, I can't currently use IDatabaseContainer and its GetConnectionString because EventStoreDbContainer doesn't implement the interface.

netlify[bot] commented 8 months ago

Deploy Preview for testcontainers-dotnet ready!

Name Link
Latest commit 32dd6158ebe2261dfd13c27312ddf0004fe02e3f
Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/655df3ed567913000834b090
Deploy Preview https://deploy-preview-1053--testcontainers-dotnet.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

alexeyzimarev commented 8 months ago

I already figured it out by looking at failing test, and I am not exactly following that decision. The interface doesn't suggest anything like that, so it's unclear from the API surface what is the intention behind that interface.

From the test perspective, and looking at other container implementations, it looks like MonoDB for example, is not a database when we all know it definitely is a database. Or Neo4j, or ESDB in that regard. I personally find it confusing.

HofmeisterAn commented 8 months ago

The interface doesn't suggest anything like that, so it's unclear from the API surface what is the intention behind that interface.

Indeed, the interface itself does not, but the description does. IIRC, the initial idea was IAdoNetDatabaseContainer or something similar.

From the test perspective, and looking at other container implementations, it looks like MonoDB for example, is not a database when we all know it definitely is a database.

No one is saying that MongoDB or ESDB are not databases, but abstraction becomes significant when commonality exists. You cannot easily swap MongoDB with ESDB as you can with ADO.NET compatible containers (AFAIK). But it is definitely ambiguous. Such as the publicly available methods to retrieve the connection string, endpoint, base address, etc. Typically, I aim to correspond to the client library.

When creating an abstraction for tests, I can't currently use IDatabaseContainer and its GetConnectionString because EventStoreDbContainer doesn't implement the interface.

Could you provide more details about what you are trying to abstract? I would like to get a better understanding of the use cases.

alexeyzimarev commented 8 months ago

I have a base class that is used across different implementation, which are database-specific. In my case, the abstraction is on a high enough level to just need the GetConnectionString be available across all those containers so I can pass the connection string to downstream implementations without exposing much other details about the container implementation.

I ended up making the base class generic, where the generic parameter is constrained to DockerContainer, and all inheritors use their own container types, and be able to call Container.GetConnectionString(). So, I had to expose container as a protected property, which I didn't want to.

HofmeisterAn commented 7 months ago

Maybe we can add support together with this issue. Right now, I am considering a connection string provider or something similar, which is capable of resolving the correct connection strings. This would allow developers to override the default connection string and could be used for the abstraction too.

HofmeisterAn commented 7 months ago

As mentioned, the interface is exclusively for ADO.NET compatible containers. I've generated a follow-up issue that includes a proposal for implementing a connection string provider across all modules. I'll close this PR. Let's carry on the discussion in the referenced issue. Thank you for bringing this to my attention.