chore: remove redundant wait.ForAll everywhere #3449
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbit
WalkthroughReplaced usages of testcontainers.WithWaitStrategy(wait.ForAll(...)) by passing the individual wait strategies directly as variadic arguments to testcontainers.WithWaitStrategy(...) across tests and many modules; no exported API signatures changed and individual waits (logs, ports, HTTP/health/exec) are preserved. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Code (call sites)
participant Run as testcontainers.Run / WithWaitStrategy
participant WaitA as Wait Strategy A
participant WaitB as Wait Strategy B
Note over Dev,Run: Previous (pre-change)
Dev->>Run: WithWaitStrategy(wait.ForAll(WaitA, WaitB))
Run->>WaitA: start
Run->>WaitB: start
Note over Dev,Run: New (post-change)
Dev->>Run: WithWaitStrategy(WaitA, WaitB)
Run->>WaitA: start
Run->>WaitB: start
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 (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 (2)
⏰ 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). (19)
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 |
![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 (1)
modules/pulsar/pulsar.go (1)
24-31: Consider removing the ForAll wrapper from defaultWaitStrategies.At line 24,
defaultWaitStrategiesis still defined usingwait.ForAll, and at line 147 it's passed directly toWithWaitStrategy(defaultWaitStrategies). For consistency with this PR's goal, consider:-var defaultWaitStrategies = wait.ForAll( - wait.ForHTTP("/admin/v2/clusters").WithPort(defaultPulsarAdminPort).WithResponseMatcher(func(r io.Reader) bool { - respBytes, _ := io.ReadAll(r) - resp := string(respBytes) - return resp == `["standalone"]` - }), - wait.ForLog("Successfully updated the policies on namespace public/default"), -) +var ( + defaultHTTPWait = wait.ForHTTP("/admin/v2/clusters").WithPort(defaultPulsarAdminPort).WithResponseMatcher(func(r io.Reader) bool { + respBytes, _ := io.ReadAll(r) + resp := string(respBytes) + return resp == `["standalone"]` + }) + defaultLogWait = wait.ForLog("Successfully updated the policies on namespace public/default") +)Then at line 147:
- testcontainers.WithWaitStrategy(defaultWaitStrategies), + testcontainers.WithWaitStrategy(defaultHTTPWait, defaultLogWait),And update lines 83 and 123:
- ss = append(ss, defaultWaitStrategies.Strategies...) + ss = append(ss, defaultHTTPWait, defaultLogWait)This would eliminate the ForAll wrapper entirely from this module.
Also applies to: 147-147
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
docker_test.go(5 hunks)modules/aerospike/aerospike.go(1 hunks)modules/artemis/artemis.go(1 hunks)modules/azure/azurite/azurite.go(1 hunks)modules/azure/eventhubs/eventhubs.go(1 hunks)modules/azure/servicebus/servicebus.go(1 hunks)modules/cassandra/cassandra.go(1 hunks)modules/chroma/chroma.go(1 hunks)modules/cockroachdb/cockroachdb.go(2 hunks)modules/consul/consul.go(1 hunks)modules/dockermcpgateway/dockermcpgateway.go(1 hunks)modules/gcloud/bigquery/bigquery.go(1 hunks)modules/gcloud/bigtable.go(1 hunks)modules/gcloud/bigtable/bigtable.go(1 hunks)modules/gcloud/datastore/datastore.go(1 hunks)modules/gcloud/firestore.go(1 hunks)modules/gcloud/firestore/firestore.go(1 hunks)modules/gcloud/pubsub.go(1 hunks)modules/gcloud/pubsub/pubsub.go(1 hunks)modules/gcloud/spanner.go(1 hunks)modules/gcloud/spanner/spanner.go(1 hunks)modules/milvus/milvus.go(2 hunks)modules/mockserver/mockserver.go(1 hunks)modules/mongodb/atlaslocal/atlaslocal.go(1 hunks)modules/mssql/mssql.go(1 hunks)modules/openfga/openfga.go(2 hunks)modules/openldap/openldap.go(1 hunks)modules/pulsar/pulsar.go(2 hunks)modules/solace/solace.go(1 hunks)network/network_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mdelapenya
PR: testcontainers/testcontainers-go#3319
File: modules/arangodb/arangodb.go:46-57
Timestamp: 2025-09-29T13:57:14.636Z
Learning: In testcontainers-go ArangoDB module, the wait strategy combines port listening check with HTTP readiness check using wait.ForAll - both strategies are required and complementary, not redundant.
📚 Learning: 2025-09-29T13:57:14.636Z
Learnt from: mdelapenya
PR: testcontainers/testcontainers-go#3319
File: modules/arangodb/arangodb.go:46-57
Timestamp: 2025-09-29T13:57:14.636Z
Learning: In testcontainers-go ArangoDB module, the wait strategy combines port listening check with HTTP readiness check using wait.ForAll - both strategies are required and complementary, not redundant.
Applied to files:
modules/dockermcpgateway/dockermcpgateway.gomodules/cockroachdb/cockroachdb.gomodules/gcloud/spanner/spanner.gomodules/cassandra/cassandra.gomodules/gcloud/spanner.gomodules/gcloud/datastore/datastore.gomodules/openfga/openfga.gomodules/azure/servicebus/servicebus.gomodules/gcloud/pubsub/pubsub.godocker_test.gomodules/milvus/milvus.gomodules/mongodb/atlaslocal/atlaslocal.gomodules/gcloud/firestore/firestore.gomodules/pulsar/pulsar.gomodules/gcloud/firestore.gomodules/artemis/artemis.gomodules/openldap/openldap.gonetwork/network_test.gomodules/mockserver/mockserver.gomodules/gcloud/bigtable.gomodules/mssql/mssql.gomodules/gcloud/bigquery/bigquery.gomodules/consul/consul.gomodules/aerospike/aerospike.gomodules/azure/azurite/azurite.go
🧬 Code graph analysis (30)
modules/dockermcpgateway/dockermcpgateway.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)
modules/cockroachdb/cockroachdb.go (1)
options.go (1)
WithWaitStrategy(366-368)
modules/gcloud/spanner/spanner.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)
modules/gcloud/bigtable/bigtable.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)
modules/cassandra/cassandra.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/host_port.go (1)
ForListeningPort(67-69)wait/exec.go (1)
ForExec(71-73)
modules/gcloud/spanner.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)
modules/gcloud/datastore/datastore.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/host_port.go (1)
ForListeningPort(67-69)wait/http.go (1)
ForHTTP(149-151)
modules/openfga/openfga.go (1)
options.go (1)
WithWaitStrategy(366-368)
modules/azure/servicebus/servicebus.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/host_port.go (1)
ForListeningPort(67-69)wait/http.go (1)
ForHTTP(149-151)
modules/gcloud/pubsub/pubsub.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)
docker_test.go (4)
options.go (1)
WithWaitStrategy(366-368)wait/log.go (1)
ForLog(118-120)wait/host_port.go (1)
ForListeningPort(67-69)wait/http.go (1)
ForHTTP(149-151)
modules/milvus/milvus.go (1)
options.go (1)
WithWaitStrategy(366-368)
modules/mongodb/atlaslocal/atlaslocal.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/host_port.go (1)
ForListeningPort(67-69)wait/health.go (1)
ForHealthCheck(55-57)
modules/azure/eventhubs/eventhubs.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/host_port.go (1)
ForListeningPort(67-69)wait/http.go (1)
ForHTTP(149-151)
modules/chroma/chroma.go (4)
options.go (1)
WithWaitStrategy(366-368)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)wait/http.go (1)
ForHTTP(149-151)
modules/gcloud/firestore/firestore.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)
modules/pulsar/pulsar.go (1)
options.go (1)
WithWaitStrategy(366-368)
modules/gcloud/firestore.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)
modules/artemis/artemis.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)
modules/gcloud/pubsub.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)
modules/openldap/openldap.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/log.go (1)
ForLog(118-120)wait/host_port.go (1)
ForListeningPort(67-69)
modules/solace/solace.go (1)
options.go (1)
WithWaitStrategy(366-368)
network/network_test.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)
modules/mockserver/mockserver.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/log.go (1)
ForLog(118-120)wait/host_port.go (1)
ForListeningPort(67-69)
modules/gcloud/bigtable.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)
modules/mssql/mssql.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)
modules/gcloud/bigquery/bigquery.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/host_port.go (1)
ForListeningPort(67-69)wait/http.go (1)
ForHTTP(149-151)
modules/consul/consul.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/log.go (1)
ForLog(118-120)wait/host_port.go (1)
ForListeningPort(67-69)
modules/aerospike/aerospike.go (3)
options.go (1)
WithWaitStrategy(366-368)wait/log.go (1)
ForLog(118-120)wait/host_port.go (1)
ForListeningPort(67-69)
modules/azure/azurite/azurite.go (1)
options.go (1)
WithWaitStrategy(366-368)
⏰ 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). (18)
- GitHub Check: lint (modules/inbucket) / lint: modules/inbucket
- GitHub Check: lint (modules/elasticsearch) / lint: modules/elasticsearch
- GitHub Check: lint (modules/redpanda) / lint: modules/redpanda
- GitHub Check: lint (modules/nats) / lint: modules/nats
- GitHub Check: lint (modules/dolt) / lint: modules/dolt
- GitHub Check: lint (modules/grafana-lgtm) / lint: modules/grafana-lgtm
- GitHub Check: lint (modules/nebulagraph) / lint: modules/nebulagraph
- GitHub Check: lint (modules/vearch) / lint: modules/vearch
- GitHub Check: lint (modules/openfga) / lint: modules/openfga
- GitHub Check: lint (modules/localstack) / lint: modules/localstack
- GitHub Check: lint (modules/azurite) / lint: modules/azurite
- GitHub Check: lint (modules/dynamodb) / lint: modules/dynamodb
- GitHub Check: lint (modules/rabbitmq) / lint: modules/rabbitmq
- GitHub Check: lint (modules/mysql) / lint: modules/mysql
- GitHub Check: lint (modules/clickhouse) / lint: modules/clickhouse
- GitHub Check: lint (modules/milvus) / lint: modules/milvus
- GitHub Check: lint (modules/pulsar) / lint: modules/pulsar
- GitHub Check: Analyze (go)
🔇 Additional comments (32)
modules/gcloud/spanner.go (1)
21-24: LGTM!The refactor correctly removes the redundant
wait.ForAllwrapper while preserving both wait conditions (port listening + log message). SinceWithWaitStrategyaccepts variadic strategies, passing them directly is functionally equivalent to wrapping them inForAll, but improves readability by reducing indirection.modules/gcloud/bigquery/bigquery.go (1)
46-51: LGTM! Correctly removes redundantwait.ForAllwrapper.The refactor preserves both wait strategies (port listening and HTTP readiness) while simplifying composition. Since
WithWaitStrategyaccepts variadic strategies that must all pass, thewait.ForAllwrapper was redundant. This improves readability and reduces indirection.modules/aerospike/aerospike.go (1)
37-42: LGTM!The removal of
wait.ForAllwrapper is correct.WithWaitStrategyalready accepts variadic strategies that must all pass, so the wrapper was redundant. The change improves readability by removing unnecessary indirection.modules/gcloud/spanner/spanner.go (1)
39-42: LGTM!The removal of
wait.ForAllwrapper is correct. Both wait strategies (port listening and log check) are now passed directly toWithWaitStrategy, which already requires all strategies to pass.modules/gcloud/pubsub.go (1)
21-24: LGTM!The removal of
wait.ForAllwrapper is correct. Even though this function is deprecated, the refactor maintains consistency and improves code clarity.modules/azure/azurite/azurite.go (1)
103-103: LGTM!The removal of
wait.ForAllwrapper is correct. The dynamically constructedwaitingForslice is now spread directly intoWithWaitStrategy, which already handles the "all must pass" semantics.modules/gcloud/firestore/firestore.go (1)
55-58: LGTM!The removal of
wait.ForAllwrapper is correct. Both wait strategies are now passed directly toWithWaitStrategy, simplifying the code without changing behavior.modules/solace/solace.go (1)
61-61: LGTM!The removal of
wait.ForAllwrapper is correct. The dynamically constructedwaitStrategiesslice (containing the primary exec check plus port-based waits for each service) is now spread directly intoWithWaitStrategy.modules/consul/consul.go (1)
64-67: LGTM!The removal of
wait.ForAllwrapper is correct. Both wait strategies (log check and port listening) are now passed directly toWithWaitStrategy, improving code clarity.modules/gcloud/bigtable/bigtable.go (1)
39-42: LGTM!The removal of
wait.ForAllwrapper is correct. Both wait strategies are now passed directly toWithWaitStrategy, which already requires all strategies to pass.modules/azure/servicebus/servicebus.go (1)
94-100: LGTM! Wait strategy composition simplified correctly.The change removes the redundant
wait.ForAllwrapper while preserving all three wait conditions (two port checks and one HTTP health check). SinceWithWaitStrategyalready requires all variadic strategies to pass, this improves readability without altering behavior.modules/milvus/milvus.go (1)
62-71: LGTM! Multi-port wait strategy simplified correctly.The change removes the
wait.ForAllwrapper while maintaining all three wait conditions (HTTP health check on port 9091, and listening checks on both HTTP and gRPC ports). All timeout and poll interval configurations are preserved.modules/chroma/chroma.go (1)
26-32: LGTM! Chroma wait strategy composition streamlined.The change removes the redundant
wait.ForAllwrapper while keeping all three wait conditions intact (port listening, log message, and HTTP heartbeat check).modules/mongodb/atlaslocal/atlaslocal.go (1)
39-39: LGTM! MongoDB Atlas Local wait strategy simplified.The change removes the
wait.ForAllwrapper, passing the port listening and health check strategies directly toWithWaitStrategy. Both conditions must still pass.modules/azure/eventhubs/eventhubs.go (1)
86-92: LGTM! Event Hubs wait strategy composition streamlined.The change removes the redundant
wait.ForAllwrapper while preserving all three wait conditions (AMQP port, HTTP port, and HTTP health endpoint). The multi-port readiness check remains intact.modules/pulsar/pulsar.go (2)
85-85: LGTM! Functions worker wait strategy unwrapped correctly.The change correctly unwraps the individual strategies from
defaultWaitStrategies.Strategiesand passes them along with the function worker log check directly toWithWaitStrategy.
125-125: LGTM! Transactions wait strategy unwrapped correctly.The change correctly unwraps the individual strategies from
defaultWaitStrategies.Strategiesand passes them along with the transaction coordinator check directly toWithWaitStrategy.modules/gcloud/bigtable.go (1)
21-24: LGTM! BigTable wait strategy simplified.The change removes the
wait.ForAllwrapper, passing the port listening and log readiness checks directly toWithWaitStrategy.modules/cassandra/cassandra.go (1)
84-90: LGTM! Cassandra wait strategy composition streamlined.The change removes the redundant
wait.ForAllwrapper while preserving both wait conditions: port listening and bootstrap completion check via cqlsh. The response matcher logic remains intact.modules/gcloud/datastore/datastore.go (1)
39-42: LGTM! Cleaner wait strategy composition.The refactor correctly removes the redundant
wait.ForAllwrapper while preserving both wait conditions (listening port and HTTP readiness). SinceWithWaitStrategyaccepts variadic strategies that all must pass, the explicitForAllwrapper was unnecessary.modules/gcloud/firestore.go (1)
21-24: LGTM! Simplified wait strategy composition.The change correctly removes the redundant
wait.ForAllwrapper while maintaining both wait conditions (listening port and log message). The behavior remains identical sinceWithWaitStrategyinherently requires all variadic strategies to pass.modules/gcloud/pubsub/pubsub.go (1)
39-42: LGTM! Cleaner wait strategy declaration.The refactor properly removes the redundant
wait.ForAllwrapper while preserving both wait conditions (listening port and log message). This improves readability without changing behavior.modules/mockserver/mockserver.go (1)
26-29: LGTM! Simplified wait strategy composition.The change correctly eliminates the redundant
wait.ForAllwrapper while maintaining both wait conditions (log message and listening port with internal check skipped). The refactor improves code clarity.modules/dockermcpgateway/dockermcpgateway.go (1)
56-59: LGTM! Cleaner wait strategy declaration.The refactor correctly removes the redundant
wait.ForAllwrapper while preserving both wait conditions (listening port and regex log pattern). This simplification improves readability without affecting behavior.modules/openfga/openfga.go (1)
55-67: LGTM! Simplified wait strategy composition.The change correctly removes the redundant
wait.ForAllwrapper while maintaining both HTTP readiness checks (health endpoint on port 8080 and playground endpoint on port 3000). The refactor improves code clarity without altering functionality.modules/mssql/mssql.go (1)
117-120: LGTM! Cleaner wait strategy declaration.The refactor properly eliminates the redundant
wait.ForAllwrapper while preserving both wait conditions (listening port with timeout and recovery completion log). This improves readability and reduces indirection during debugging.modules/openldap/openldap.go (1)
121-124: LGTM! Simplified wait strategy composition.The change correctly removes the redundant
wait.ForAllwrapper while preserving both required wait conditions (slapd startup log and listening port). SinceWithWaitStrategyaccepts variadic strategies that all must pass, both conditions remain complementary and required as intended. Based on learnings.modules/artemis/artemis.go (1)
106-111: LGTM! Cleaner wait strategy composition.The removal of
wait.ForAllreduces indirection while preserving the same wait semantics.WithWaitStrategyalready ensures all strategies must pass.modules/cockroachdb/cockroachdb.go (1)
194-208: LGTM! Simplified wait strategy composition.The change correctly removes the redundant
wait.ForAllwrapper while preserving all four wait conditions (file, HTTP health check, TLS cert, and SQL readiness). This improves code clarity without altering behavior.network/network_test.go (1)
143-146: LGTM! Consistent with repository-wide refactor.Removing the
wait.ForAllwrapper simplifies the wait strategy composition while maintaining the same behavior (waiting for both port and log).docker_test.go (2)
607-610: LGTM! Consistent refactor across test suite.The removal of
wait.ForAllwrapper aligns with the repository-wide changes and maintains the same wait behavior.
649-651: LGTM! Simplified single-strategy wait conditions.Removing the
wait.ForAllwrapper even for single strategies improves consistency and eliminates unnecessary nesting. The variadicWithWaitStrategyhandles both single and multiple strategies uniformly.Also applies to: 668-670, 687-689, 1434-1440
NathanBaulch
force-pushed
the
waitforall
branch
from
f83c81a to
abd8969
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR removes redundant wait.ForAll wrapper calls when using WithWaitStrategy, since the function already accepts a variadic slice of strategies and waits for all of them to pass. This change improves code readability by eliminating unnecessary indirection.
- Removes
wait.ForAllwrapper from multipleWithWaitStrategycalls across test and module files - Converts variable declarations from
wait.ForAll()to slice literals where applicable - Updates function calls to pass strategies directly as variadic arguments
Reviewed Changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| network/network_test.go | Removes wait.ForAll wrapper in test configuration |
| modules/*/*.go | Removes redundant wait.ForAll calls in 25+ module files |
| docker_test.go | Updates test cases to use direct strategy passing |
| modules/pulsar/pulsar.go | Changes variable type from wait.ForAll to slice and updates usage |
| modules/neo4j/neo4j.go | Replaces wait.MultiStrategy with direct strategy passing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
LGTM! I was actually thinking about this refactor, as I noticed the initial pass of the "use Run function" PRs added it, although the latter ones did remove it, so thank you so much for anticipating this work.
2 participants
What does this PR do?
Removes redundant
wait.ForAllcalls passed toWithWaitStrategy, which already takes a variadic slice of strategies that must all pass.Why is it important?
Improves readability and reduces indirection when debugging.