chore: use Run function (part 3) #3307
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbit
WalkthroughMigrates tests from GenericContainer/GenericContainerRequest to Run(ctx, image, opts...) with ContainerCustomizer options, adjusts assertions to require, and updates networking test configurations. Adds a host-networking test and changes a docs reference to a new test file path. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester as Test
participant TC as testcontainers.Run
participant Opts as ContainerCustomizer opts
participant Docker as Docker Daemon
participant C as Container
rect rgba(200,230,255,0.3)
note right of Tester: Build options (ports, files, wait, network)
Tester->>TC: Run(ctx, image, opts...)
TC->>Opts: Apply WithExposedPorts / WithFiles / WithWaitStrategy / network.*
end
TC->>Docker: CreateContainer(config, hostConfig, networking)
Docker-->>TC: Container ID
TC->>Docker: StartContainer(ID)
rect rgba(220,255,220,0.3)
TC->>Docker: WaitStrategy checks (logs/ports/health)
Docker-->>TC: Ready/Timeout/Error
end
alt Ready
TC-->>Tester: Container handle (ID, host/ports)
Tester->>C: Exec/Inspect assertions
else Timeout/Error
TC-->>Tester: Error (propagates to require.*)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
network/network_test.go (1)
436-458: Consider improving error handling and test organization.The test function has a few areas for improvement:
- The error handling could be more precise - consider asserting on the specific error type
- The comment on line 456 could be clearer about what error is expected
- if err != nil { - // Error when NetworkMode = host and Network = []string{"bridge"} - t.Logf("Can't use Network and NetworkMode together, %s\n", err) - } + // When using host network mode with a named network, Docker returns an error + // as these configurations are mutually exclusive + require.Error(t, err, "Expected error when combining host network mode with named network") + require.Contains(t, err.Error(), "network", "Error should indicate network configuration conflict")docker_test.go (1)
373-387: Consider extracting the bridge network option to a test helper.Since
withBridgeNetworkis defined inline and might be useful in other tests, consider extracting it to a test helper function or using the existingnetwork.WithBridgeNetworkif available.Based on the learnings, it appears
network.WithBridgeNetworkalready exists. Consider using it instead of defining an anonymous function:- // avoid cyclic import with the network package by defining the anonymous function here - withBridgeNetwork := func() CustomizeRequestOption { - return func(req *GenericContainerRequest) error { - req.Networks = append(req.Networks, "bridge") - return nil - } - } - nginxC, err := Run( ctx, nginxAlpineImage, WithExposedPorts(nginxDefaultPort), WithWaitStrategy(wait.ForHTTP("/").WithPort(nginxDefaultPort)), WithName(creationName), - withBridgeNetwork(), + network.WithBridgeNetwork(), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docker_test.go(60 hunks)docs/features/networking.md(1 hunks)network/network_test.go(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
network/network_test.go (4)
generic.go (1)
Run(122-149)options.go (4)
WithExposedPorts(464-469)WithWaitStrategy(376-378)ContainerCustomizer(22-24)WithHostConfigModifier(88-94)network/network.go (4)
WithNetwork(139-141)WithBridgeNetwork(167-172)New(22-56)WithNetworkName(147-163)testing.go (3)
CleanupNetwork(103-111)CleanupContainer(91-97)SkipIfDockerDesktop(43-54)
docker_test.go (4)
options.go (18)
ContainerCustomizer(22-24)WithExposedPorts(464-469)WithFiles(534-539)WithWaitStrategy(376-378)WithHostConfigModifier(88-94)WithLogConsumerConfig(280-285)WithDockerfile(47-53)CustomizeRequestOption(28-28)WithName(109-117)WithEnv(75-85)WithCmd(472-477)WithEntrypoint(448-453)WithConfigModifier(56-62)WithMounts(515-520)WithTmpfs(523-531)WithImagePlatform(440-445)WithNoStart(120-125)WithImageSubstitutors(256-262)container.go (2)
ContainerFile(110-115)FromDockerfile(90-108)generic.go (2)
Run(122-149)GenericContainerRequest(21-27)testing.go (1)
CleanupContainer(91-97)
⏰ 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). (17)
- GitHub Check: lint (modules/surrealdb) / lint: modules/surrealdb
- GitHub Check: lint (modules/mongodb) / lint: modules/mongodb
- GitHub Check: lint (modules/yugabytedb) / lint: modules/yugabytedb
- GitHub Check: lint (modules/kafka) / lint: modules/kafka
- GitHub Check: lint (modules/consul) / lint: modules/consul
- GitHub Check: lint (modules/redis) / lint: modules/redis
- GitHub Check: lint (modules/weaviate) / lint: modules/weaviate
- GitHub Check: lint (modules/mssql) / lint: modules/mssql
- GitHub Check: lint (modules/nebulagraph) / lint: modules/nebulagraph
- GitHub Check: lint (modules/registry) / lint: modules/registry
- GitHub Check: lint (modulegen) / lint: modulegen
- GitHub Check: lint (modules/gcloud) / lint: modules/gcloud
- GitHub Check: lint (modules/mockserver) / lint: modules/mockserver
- GitHub Check: lint (modules/vault) / lint: modules/vault
- GitHub Check: lint (modules/influxdb) / lint: modules/influxdb
- GitHub Check: lint (modules/socat) / lint: modules/socat
- GitHub Check: Analyze (go)
🔇 Additional comments (40)
docs/features/networking.md (1)
132-132: LGTM! Documentation reference correctly updated.The documentation path update from
docker_test.gotonetwork/network_test.goproperly reflects the file reorganization where theSkipIfDockerDesktopexample has been moved to the network package.network/network_test.go (9)
5-5: LGTM! Import addition for host networking test.The
osimport is appropriately added for the environment variable check in the newTestContainerWithNetworkModeAndNetworkTogethertest.
9-9: LGTM! Required import for host configuration.The
containertypes import is necessary for usingcontainer.HostConfigin the new networking tests.
40-43: LGTM! Clean migration to Run pattern.The migration from
GenericContainertoRunwith functional options improves readability and follows the new pattern consistently.
78-81: LGTM! Consistent use of Run with network options.The test properly demonstrates using
Runwith multiple network aliases through thenetwork.WithNetworkoption.
117-122: LGTM! Multiple network configuration properly migrated.The test correctly combines multiple network options (
WithNetworkandWithBridgeNetwork) showing the flexibility of the new API.
141-156: LGTM! Dynamic option building pattern well implemented.The test demonstrates a good pattern for building container options dynamically, appending network configurations in a loop before passing to
Run.
184-186: LGTM! Concise network configuration.Clean migration showing simplified container creation with network attachment.
190-192: LGTM! Consistent network setup for second container.Properly demonstrates multiple containers joining the same network using the new API.
237-240: LGTM! Network configuration with IPAM settings.The test properly migrates to
Runwhile maintaining complex network configurations like IPAM.docker_test.go (30)
14-14: LGTM! Import for numeric conversions in tests.The
strconvimport is appropriately added for the new test naming pattern usingstrconv.FormatBool.
38-38: LGTM! Consistent constant naming.The addition of
alpineImageconstant follows the existing naming pattern and provides consistency across tests.
64-77: LGTM! Clean migration with configuration modifiers.The test properly demonstrates using
WithHostConfigModifierfor network mode configuration alongside file mounting and wait strategies.
91-97: LGTM! Simplified container configuration.Clean migration showing how
WithHostConfigModifiercan be used for privilege configuration.
124-135: LGTM! Host network configuration properly migrated.The test maintains the same functionality while using the new
Runpattern with functional options.
155-159: LGTM! Simple container creation migrated.Clean and concise migration to the
Runfunction.
178-184: LGTM! Log consumer configuration migrated.Proper use of
WithLogConsumerConfigin the new pattern.
207-207: Consistent test naming improvement.Good update to use more descriptive test names with the "after-termination" prefix.
256-256: LGTM! Clear test case naming.The updated test names "not-built-from-Dockerfile" and "built-from-Dockerfile" are more descriptive.
Also applies to: 273-273
279-286: LGTM! FromDockerfile pattern properly migrated.Clean migration showing how to build containers from Dockerfiles using the new API.
513-524: LGTM! Build args pattern properly demonstrated.Clean migration showing how to pass build arguments when building from Dockerfile.
647-652: LGTM! Command override pattern migrated.Proper use of
WithCmdto override container command.
666-671: LGTM! Entrypoint override pattern migrated.Proper use of
WithEntrypointto override container entrypoint.
685-693: LGTM! Config modifier for working directory.Clean demonstration of using
WithConfigModifierto set the working directory.
976-991: LGTM! Improved test case naming and structure.The reorganized test cases with descriptive names and proper error handling patterns are well structured.
997-997: Good use of descriptive test names.The test names "non-existent-platform" and "valid-platform" clearly indicate what's being tested.
Also applies to: 1007-1007
1080-1085: LGTM! Consistent test naming pattern.The updated test names use consistent hyphen-separated format improving readability.
1138-1157: LGTM! Clear test case structure.Well-structured test cases with descriptive names and proper file mode handling.
1206-1241: LGTM! Comprehensive directory copy test cases.Well-organized test cases covering various scenarios with clear naming.
1271-1277: LGTM! Consistent test naming.The test names follow the established pattern with clear descriptions.
1375-1383: LGTM! Resource configuration properly migrated.Clean use of
WithHostConfigModifierfor setting resource limits.
1408-1414: LGTM! Capability configuration migrated.Proper use of
WithHostConfigModifierfor adding capabilities.
1431-1441: LGTM! Complex wait strategy properly migrated.Good example of using multiple wait strategies with status code matching.
1450-1456: LGTM! User configuration properly migrated.Clean use of
WithConfigModifierto set container user.
1511-1515: LGTM! Network mode configuration migrated.Proper use of
WithHostConfigModifierfor container network mode.
1607-1607: LGTM! Use of strconv for test naming.Good use of
strconv.FormatBoolfor generating test names from boolean values.
1615-1621: LGTM! KeepImage option properly demonstrated.Clean migration showing how to control image retention after container termination.
1750-1752: LGTM! Improved assertions.Good use of
require.Positivefor clearer test assertions.
1801-1803: LGTM! Consistent assertion improvements.Consistent use of improved assertions throughout the retry tests.
1862-1864: LGTM! Consistent assertion pattern.Maintains consistent assertion style across all retry test cases.
1 participant
What does this PR do?
Use the Run function in the core tests and in the network package's tests.
Why is it important?
Make progress towards using the new API
Related issues