chore: Set remaining container image explicit #1606
Conversation
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughStandardizes explicit image specification across multiple builders and tests by adding image-accepting constructors ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-11-17T17:58:43.958ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@HofmeisterAn |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Testcontainers.ServiceBus/ServiceBusBuilder.cs (1)
145-156: UseMsSqlBuilder.MsSqlImageconstant instead of hardcoding the image string.The MSSQL image tag is duplicated in
ServiceBusBuilder.cs(line 150) andServiceBusContainerTest.cs(line 119). A publicMsSqlImageconstant already exists inMsSqlBuilder, so reference it directly or define a local constant to maintain consistency. This avoids drift when the image tag is updated inMsSqlBuilder.- var container = new MsSqlBuilder("mcr.microsoft.com/mssql/server:2022-CU14-ubuntu-22.04") + var container = new MsSqlBuilder(MsSqlBuilder.MsSqlImage)Apply the same fix to the test file.
tests/Testcontainers.ServiceBus.Tests/ServiceBusContainerTest.cs (1)
114-123: Implement IAsyncDisposable to clean up Network and Container resources.The
DatabaseFixturecreates anINetworkandMsSqlContainer, both of which implementIAsyncDisposableand require cleanup. Without implementingIAsyncDisposableon the fixture, xUnit will not callDisposeAsync(), causing Docker resources to leak.Add the disposal pattern:
public sealed class DatabaseFixture : IAsyncDisposable { // ... existing code ... public async ValueTask DisposeAsync() { if (Container != null) await Container.DisposeAsync().ConfigureAwait(false); if (Network != null) await Network.DisposeAsync().ConfigureAwait(false); } }Optionally, extract the MSSQL image tag to a constant to avoid duplication.
🧹 Nitpick comments (4)
tests/Testcontainers.Tests/Unit/Containers/Unix/TestcontainersContainerTest.cs (1)
21-26: Remove commented code instead of leaving it in the codebase.While the explanatory comment is helpful, the commented-out test should be removed entirely. Git history preserves the original test if future reference is needed, and keeping commented code can create confusion about whether it should eventually be restored.
Apply this diff to remove the commented code:
- // This test will always pass, because we always set Image inside builder constructor. - // [Fact] - // public void ShouldThrowArgumentNullExceptionWhenBuildConfigurationHasNoImage() - // { - // Assert.Throws<ArgumentException>(() => _ = new ContainerBuilder(CommonImages.Alpine).Build()); - // } -tests/Testcontainers.Tests/Fixtures/Containers/Unix/DockerMTls.cs (1)
9-10: Good change - aligns with explicit image specification pattern.The constructor now explicitly specifies the Docker image instead of relying on defaults, which improves clarity and reproducibility. The image specification (
"docker", null registry,dockerImageVersion + "-dind"tag) is consistent with the pattern adopted in related fixtures.Optional: Consider adding null/empty validation for the
dockerImageVersionparameter to prevent constructing an invalid tag like"-dind", though this may be handled upstream.tests/Testcontainers.ServiceBus.Tests/DeclineLicenseAgreementTest.cs (1)
12-12: Avoid duplicating the emulator image string (and consider not using:latestfor test stability).
A small cleanup is to extract the image into a single constant; also,:latestcan make these tests flaky if the upstream image changes its EULA/error behavior.public sealed partial class DeclineLicenseAgreementTest { + private const string ServiceBusEmulatorImage = "mcr.microsoft.com/azure-messaging/servicebus-emulator:latest"; + [GeneratedRegex("The image '.+' requires you to accept a license agreement\\.")] private static partial Regex LicenseAgreementNotAccepted(); @@ public void WithoutAcceptingLicenseAgreementThrowsArgumentException() { - var exception = Assert.Throws<ArgumentException>(() => new ServiceBusBuilder("mcr.microsoft.com/azure-messaging/servicebus-emulator:latest").Build()); + var exception = Assert.Throws<ArgumentException>(() => new ServiceBusBuilder(ServiceBusEmulatorImage).Build()); Assert.Matches(LicenseAgreementNotAccepted(), exception.Message); } @@ public void WithLicenseAgreementDeclinedThrowsArgumentException() { - var exception = Assert.Throws<ArgumentException>(() => new ServiceBusBuilder("mcr.microsoft.com/azure-messaging/servicebus-emulator:latest").WithAcceptLicenseAgreement(false).Build()); + var exception = Assert.Throws<ArgumentException>(() => new ServiceBusBuilder(ServiceBusEmulatorImage).WithAcceptLicenseAgreement(false).Build()); Assert.Matches(LicenseAgreementNotAccepted(), exception.Message); } }Also applies to: 20-20
tests/Testcontainers.Tests/Fixtures/Containers/Unix/DockerTlsFixture.cs (1)
12-13: Version 29.0.0-dind is valid, but consider the breaking changes introduced in Docker 29.0.0.The explicit image pinning is sound. Docker version 29.0.0-dind is an official release and available on Docker Hub. However, Docker Engine 29.0.0 raises the minimum client/API version, which is a breaking change that could impact test compatibility. If this fixture is intended to test with the latest release, the current version is appropriate; otherwise, consider using a pre-29 version (e.g., 28.x) to avoid potential compatibility issues with clients or runners.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/Testcontainers.EventHubs/EventHubsBuilder.cs(1 hunks)src/Testcontainers.ServiceBus/ServiceBusBuilder.cs(1 hunks)src/Testcontainers/Containers/SocatBuilder.cs(1 hunks)tests/Testcontainers.EventHubs.Tests/EventHubsContainerTest.cs(1 hunks)tests/Testcontainers.Platform.Linux.Tests/ConnectionStringProviderTest.cs(2 hunks)tests/Testcontainers.Platform.Linux.Tests/SocatContainerTest.cs(1 hunks)tests/Testcontainers.ServiceBus.Tests/DeclineLicenseAgreementTest.cs(1 hunks)tests/Testcontainers.ServiceBus.Tests/ServiceBusContainerTest.cs(1 hunks)tests/Testcontainers.Tests/Fixtures/Containers/Unix/DockerMTls.cs(1 hunks)tests/Testcontainers.Tests/Fixtures/Containers/Unix/DockerTlsFixture.cs(1 hunks)tests/Testcontainers.Tests/Fixtures/Containers/Unix/ProtectDockerDaemonSocket.cs(1 hunks)tests/Testcontainers.Tests/Unit/Containers/Unix/TestcontainersContainerTest.cs(1 hunks)tests/Testcontainers.Toxiproxy.Tests/ToxiproxyContainerTest.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-17T17:58:43.958Z
Learnt from: diegosasw
Repo: testcontainers/testcontainers-dotnet PR: 1583
File: src/Testcontainers.KurrentDb/Testcontainers.KurrentDb.csproj:7-7
Timestamp: 2025-11-17T17:58:43.958Z
Learning: In the testcontainers-dotnet repository, JetBrains.Annotations should use version 2023.3.0 to maintain consistency with the main Testcontainers csproj, rather than always using the latest available version.
Applied to files:
tests/Testcontainers.Platform.Linux.Tests/SocatContainerTest.cssrc/Testcontainers.EventHubs/EventHubsBuilder.cstests/Testcontainers.ServiceBus.Tests/ServiceBusContainerTest.cstests/Testcontainers.Toxiproxy.Tests/ToxiproxyContainerTest.cstests/Testcontainers.Tests/Fixtures/Containers/Unix/DockerMTls.cstests/Testcontainers.Tests/Unit/Containers/Unix/TestcontainersContainerTest.cstests/Testcontainers.Tests/Fixtures/Containers/Unix/ProtectDockerDaemonSocket.cstests/Testcontainers.Tests/Fixtures/Containers/Unix/DockerTlsFixture.cstests/Testcontainers.Platform.Linux.Tests/ConnectionStringProviderTest.cstests/Testcontainers.EventHubs.Tests/EventHubsContainerTest.cssrc/Testcontainers.ServiceBus/ServiceBusBuilder.cs
🧬 Code graph analysis (7)
tests/Testcontainers.Platform.Linux.Tests/SocatContainerTest.cs (1)
src/Testcontainers/Containers/SocatBuilder.cs (9)
SocatBuilder(39-42)SocatBuilder(54-58)SocatBuilder(64-68)SocatBuilder(79-82)SocatBuilder(91-96)SocatBuilder(118-123)SocatBuilder(137-140)SocatBuilder(143-146)SocatBuilder(149-152)
src/Testcontainers.EventHubs/EventHubsBuilder.cs (1)
src/Testcontainers.Azurite/AzuriteBuilder.cs (9)
AzuriteBuilder(22-27)AzuriteBuilder(48-51)AzuriteBuilder(61-65)AzuriteBuilder(71-75)AzuriteBuilder(88-98)AzuriteBuilder(127-135)AzuriteBuilder(138-141)AzuriteBuilder(144-147)AzuriteBuilder(150-153)
tests/Testcontainers.Tests/Fixtures/Containers/Unix/DockerMTls.cs (1)
tests/Testcontainers.Tests/Fixtures/Containers/Unix/ProtectDockerDaemonSocket.cs (2)
ProtectDockerDaemonSocket(14-98)ProtectDockerDaemonSocket(26-34)
src/Testcontainers/Containers/SocatBuilder.cs (3)
src/Testcontainers.EventHubs/EventHubsBuilder.cs (1)
Obsolete(21-25)src/Testcontainers.ServiceBus/ServiceBusBuilder.cs (1)
Obsolete(21-25)src/Testcontainers.Weaviate/WeaviateBuilder.cs (1)
Obsolete(17-18)
tests/Testcontainers.Tests/Fixtures/Containers/Unix/ProtectDockerDaemonSocket.cs (1)
src/Testcontainers/Builders/ContainerBuilder.cs (7)
ContainerBuilder(52-55)ContainerBuilder(62-66)ContainerBuilder(72-76)ContainerBuilder(89-92)ContainerBuilder(95-98)ContainerBuilder(101-104)ContainerBuilder(107-110)
tests/Testcontainers.Platform.Linux.Tests/ConnectionStringProviderTest.cs (1)
src/Testcontainers/Builders/ContainerBuilder.cs (1)
IContainer(82-86)
src/Testcontainers.ServiceBus/ServiceBusBuilder.cs (1)
src/Testcontainers.MsSql/MsSqlBuilder.cs (10)
MsSqlBuilder(37-40)MsSqlBuilder(52-56)MsSqlBuilder(62-66)MsSqlBuilder(76-81)MsSqlBuilder(91-100)MsSqlBuilder(113-116)MsSqlBuilder(119-122)MsSqlBuilder(125-128)MsSqlBuilder(138-142)MsSqlBuilder(152-156)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: ci (Testcontainers.ActiveMq, ubuntu-24.04)
- GitHub Check: ci (Testcontainers.Cassandra, ubuntu-24.04)
- GitHub Check: ci (Testcontainers.Bigtable, ubuntu-24.04)
- GitHub Check: ci (Testcontainers.BigQuery, ubuntu-24.04)
- GitHub Check: ci (Testcontainers.ArangoDb, ubuntu-24.04)
- GitHub Check: ci (Testcontainers.Azurite, ubuntu-24.04)
- GitHub Check: analyze (csharp)
🔇 Additional comments (12)
tests/Testcontainers.Tests/Fixtures/Containers/Unix/DockerMTls.cs (1)
5-5: LGTM! Necessary import for explicit image specification.The addition of the
DotNet.Testcontainers.Imagesnamespace is required to use theDockerImageclass in the constructor.src/Testcontainers.EventHubs/EventHubsBuilder.cs (1)
145-145: LGTM! Explicit image pin improves reproducibility.The explicit image specification for the default Azurite container correctly follows the standardization pattern and ensures consistent behavior when users don't provide their own Azurite container instance.
tests/Testcontainers.Tests/Fixtures/Containers/Unix/DockerTlsFixture.cs (1)
5-5: LGTM: Necessary import for DockerImage.The addition of the
DotNet.Testcontainers.Imagesnamespace is required for theDockerImagetype used in the constructor.tests/Testcontainers.Platform.Linux.Tests/ConnectionStringProviderTest.cs (1)
15-15: LGTM: Consistent image specification across test classes.Both test classes now explicitly specify
CommonImages.Alpineinstead of relying on a parameterless constructor. This aligns with the image pinning standardization from PR #1584 and maintains consistency across the test suite.Also applies to: 44-44
tests/Testcontainers.Tests/Fixtures/Containers/Unix/ProtectDockerDaemonSocket.cs (1)
26-34: Constructor parameter change is intentional and properly implemented across subclasses.The constructor signature change—removing the
dockerImageVersionparameter and delegating image specification to callers—is a deliberate design pattern that has been correctly implemented. BothDockerMTlsandDockerTlsFixtureproperly provide an image-configuredContainerBuilderto the base constructor before calling it. This pattern shifts image responsibility to the direct subclass level, which is a sound design choice.Since the constructor is
protected(not public), this only affects subclasses, and all known subclasses in the codebase are updated correctly. However, if custom subclasses exist outside this file, they will need to follow the same pattern: configure the image in theContainerBuilderbefore passing it to the base constructor.The PR title's reference to "adding image pins" is consistent with this change—the image versions are now pinned at the subclass level (e.g.,
"29.0.0-dind"inDockerTlsFixture), rather than being a parameter passed through the base class.tests/Testcontainers.Platform.Linux.Tests/SocatContainerTest.cs (1)
23-23: LGTM! Explicit image specification improves test clarity.The update correctly uses the new constructor signature with an explicit image tag that matches the previous default (
SocatImageconstant). This makes the test more explicit and aligns with the repository-wide pattern from PR #1584.src/Testcontainers/Containers/SocatBuilder.cs (6)
9-9: LGTM! Required namespace for image types.The
DotNet.Testcontainers.Imagesnamespace is necessary to support the newIImageandDockerImagetypes used in the constructor overloads.
16-17: LGTM! Appropriate deprecation with migration guidance.The obsolete attribute correctly guides users to the new constructor pattern while maintaining backward compatibility. The constant is still used internally by the obsolete parameterless constructor and
Init()method.
22-27: LGTM! Proper deprecation maintaining backward compatibility.The obsolete parameterless constructor correctly delegates to the private constructor and uses
Init()to apply the default image. This maintains backward compatibility while encouraging migration to the new constructors.
29-42: LGTM! Well-documented constructor following established pattern.The string-based constructor correctly delegates to the
IImage-based constructor throughDockerImage. The documentation is comprehensive and includes a helpful link to available Docker images.
44-58: LGTM! Correctly implements image override pattern.The
IImageconstructor properly initializes the configuration and overrides the default image through theInit().WithImage(image)chain. This ensures the user-specified image takes precedence over the default.
118-123: The obsolete parameterless constructor has been properly deprecated with [Obsolete] attributes and removed from all call sites in the codebase. TheInit()method correctly uses theSocatImageconstant to provide backward compatibility for the deprecated constructor, while new constructors with explicit image parameters properly override this behavior. No migration work remains.
HofmeisterAn
added
the
chore
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/Testcontainers.ServiceBus.Tests/DeclineLicenseAgreementTest.cs (1)
10-14: Good: tests now explicitly select the Service Bus image (more deterministic).Minor: consider hoisting
TestSession.GetImageFromDockerfile()into a localimagevariable shared by both tests to reduce duplication and make Dockerfile-resolution failures easier to diagnose.Also applies to: 18-22
src/Testcontainers/Containers/SocatBuilder.cs (1)
117-122: Consider refactoring to avoid obsolete constant usage.The
Init()method still references the obsoleteSocatImageconstant on line 120, which will generate a compiler warning. While this works correctly because the new constructors override the image afterward, consider one of these approaches:
- Suppress the warning with
#pragma warning disableif this is intentional- Refactor to avoid setting a default image in
Init()when called from the new constructorsHowever, if this pattern was established in PR #1584 and is consistent across other builders, it may be acceptable as-is for backward compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Testcontainers/Containers/SocatBuilder.cs(1 hunks)tests/Testcontainers.Platform.Linux.Tests/SocatContainerTest.cs(1 hunks)tests/Testcontainers.ServiceBus.Tests/DeclineLicenseAgreementTest.cs(1 hunks)tests/Testcontainers.Tests/Fixtures/Containers/Unix/DockerMTls.cs(1 hunks)tests/Testcontainers.Tests/Fixtures/Containers/Unix/DockerTlsFixture.cs(1 hunks)tests/Testcontainers.Tests/Unit/Containers/Unix/TestcontainersContainerTest.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Testcontainers.Tests/Fixtures/Containers/Unix/DockerMTls.cs
- tests/Testcontainers.Platform.Linux.Tests/SocatContainerTest.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-17T17:58:43.958Z
Learnt from: diegosasw
Repo: testcontainers/testcontainers-dotnet PR: 1583
File: src/Testcontainers.KurrentDb/Testcontainers.KurrentDb.csproj:7-7
Timestamp: 2025-11-17T17:58:43.958Z
Learning: In the testcontainers-dotnet repository, JetBrains.Annotations should use version 2023.3.0 to maintain consistency with the main Testcontainers csproj, rather than always using the latest available version.
Applied to files:
tests/Testcontainers.Tests/Fixtures/Containers/Unix/DockerTlsFixture.cssrc/Testcontainers/Containers/SocatBuilder.cstests/Testcontainers.Tests/Unit/Containers/Unix/TestcontainersContainerTest.cs
🧬 Code graph analysis (3)
tests/Testcontainers.ServiceBus.Tests/DeclineLicenseAgreementTest.cs (2)
src/Testcontainers.ServiceBus/ServiceBusBuilder.cs (10)
ServiceBusBuilder(37-40)ServiceBusBuilder(52-56)ServiceBusBuilder(62-66)ServiceBusBuilder(88-92)ServiceBusBuilder(107-119)ServiceBusBuilder(129-132)ServiceBusBuilder(160-168)ServiceBusBuilder(171-174)ServiceBusBuilder(177-180)ServiceBusBuilder(183-186)tests/Testcontainers.Commons/TestSession.cs (2)
TestSession(10-13)GetImageFromDockerfile(15-27)
tests/Testcontainers.Tests/Fixtures/Containers/Unix/DockerTlsFixture.cs (1)
tests/Testcontainers.Tests/Fixtures/Containers/Unix/ProtectDockerDaemonSocket.cs (2)
ProtectDockerDaemonSocket(14-98)ProtectDockerDaemonSocket(26-34)
src/Testcontainers/Containers/SocatBuilder.cs (3)
tests/Testcontainers.Commons/CommonImages.cs (1)
PublicAPI(3-19)src/Testcontainers/Containers/SocatContainer.cs (2)
PublicAPI(6-17)SocatContainer(13-16)src/Testcontainers/Containers/SocatConfiguration.cs (6)
PublicAPI(10-68)SocatConfiguration(17-21)SocatConfiguration(27-31)SocatConfiguration(37-41)SocatConfiguration(47-51)SocatConfiguration(58-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: ci (Testcontainers.ActiveMq, ubuntu-24.04)
- GitHub Check: ci (Testcontainers.Bigtable, ubuntu-24.04)
- GitHub Check: ci (Testcontainers.Azurite, ubuntu-24.04)
- GitHub Check: ci (Testcontainers.ArangoDb, ubuntu-24.04)
- GitHub Check: ci (Testcontainers.BigQuery, ubuntu-24.04)
- GitHub Check: ci (Testcontainers.Cassandra, ubuntu-24.04)
- GitHub Check: analyze (csharp)
🔇 Additional comments (5)
tests/Testcontainers.Tests/Fixtures/Containers/Unix/DockerTlsFixture.cs (1)
11-15: Docker image pin is valid and aligns with PR objectives.The change successfully pins the Docker image to version 29.0.0-dind, making the version explicit and standardizing image specification across builders. The image version has been verified to exist on Docker Hub.
src/Testcontainers/Containers/SocatBuilder.cs (4)
9-9: LGTM! Necessary namespace for image handling.The
DotNet.Testcontainers.Imagesnamespace is correctly added to support the newIImageandDockerImagetypes used in the constructors.
22-26: LGTM! Clean deprecation pattern.The parameterless constructor properly delegates to the new string-based constructor while maintaining backward compatibility. The obsolete attribute with reference link helps users migrate to the new pattern.
28-41: LGTM! Well-documented constructor.The string-based constructor is properly implemented with clear documentation and helpful links. The delegation to the
IImageconstructor is clean and follows the builder pattern effectively.
43-57: LGTM! Constructor correctly configures the image.The
IImageconstructor properly initializes the builder and applies the provided image. The implementation correctly callsInit().WithImage(image)to ensure the image is set after base initialization, which works as intended even thoughInit()internally sets a default image first.
tests/Testcontainers.Tests/Unit/Containers/Unix/TestcontainersContainerTest.cs
Show resolved
Hide resolved
HofmeisterAn
changed the title
2 participants
What does this PR do?
Fix some Builders we missed in PR #1584
Why is it important?
This should have been part of PR #1584
Related issues
#1584
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.