Repository navigation
feat(wait): backport ForAny and wait cleanups from testcontainers-go - #171
Merged
Merged
Conversation
…iners/testcontainers-go#3208) Apply gopls modernize tool with fix and format code. Backport note: only the container/wait part (walk.go: slices.Delete) is ported; the conflict is resolved to keep go-sdk's ErrVisitStop. Backported-from: testcontainers/testcontainers-go@42824ba
Partial backport of testcontainers/testcontainers-go#3568 ("chore(compose): update to compose-v5"): only the wait/errors.go change, removing the legacy "// +build !windows" line (the //go:build line remains). Backported-from: testcontainers/testcontainers-go@d2d0d10
…estcontainers/testcontainers-go#3719) * feat(wait): implement AnyMultiStrategy: ForAny equivalent to ForAll. Upstream issue: testcontainers/testcontainers-go#3717. * fix: lint Signed-off-by: mdelapenya <mdelapenya@gmail.com> * fix: formatting Signed-off-by: mdelapenya <mdelapenya@gmail.com> --------- Backport notes (go-sdk adaptations): - walk.go: keep go-sdk's ErrVisitStop (the deprecated VisitStop alias was removed) - tests: use &NopStrategyTarget{} (pointer receivers in go-sdk), WithTimeout instead of WithStartupTimeout, typed network.Port in ForListeningPort - examples: import github.com/docker/go-sdk/container/wait - TestAnyMultiStrategy_WaitsForAny keeps its testing/synctest.Test usage without a //go:build go1.25 constraint: go-sdk is on Go 1.27 - docs/ changes not ported (go-sdk has no docs site) Signed-off-by: mdelapenya <mdelapenya@gmail.com> Co-authored-by: mdelapenya <mdelapenya@gmail.com> Backported-from: testcontainers/testcontainers-go@96ab095
…ry mocks (testcontainers/testcontainers-go#3603) * chore(wait): apply review suggestions to exit_test and health_test - Switch to package wait_test - Replace hand-written mocks with mockStrategyTarget + typed EXPECT() chains - Use anyContext matcher for context args - Use moby/moby import paths (not docker/docker) - Use typed container state constants - Extract newStateTarget() helper in health_test.go - Replace goroutine+mutex pattern in TestWaitForHealthWithNil with RunAndReturn closure Addresses review feedback on PR testcontainers/testcontainers-go#3603. * test(wait): add t.Helper() to newStateTarget Satisfies the thelper linter; newStateTarget is a test helper. Signed-off-by: Mateen Anjum <mateenali66@gmail.com> --------- Backport notes (go-sdk adaptations): import github.com/docker/go-sdk/container/wait, use WithTimeout instead of WithExitTimeout/WithStartupTimeout, and drop the go-sdk-only healthStrategyTarget.Logger() helper along with the hand-written target. Signed-off-by: Mateen Anjum <mateenali66@gmail.com> Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com> Backported-from: testcontainers/testcontainers-go@eb511c4
mdelapenya
marked this pull request as ready for review
October 23, 2026 10:38
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5b6081ae7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
reflect.Value.IsNil panics for non-nil-capable kinds such as structs, so
ForAny(valueStrategy{}) crashed in WaitUntilReady and String. Guard the
reflected kind before calling IsNil and add a regression test.
Follow-up to the testcontainers/testcontainers-go#3719 backport; addresses
review feedback on PR #171. The same pattern exists in MultiStrategy (all.go)
and is left unchanged here.
robmry
previously approved these changes
Oct 23, 2026
robmry
left a comment
robmry
left a comment
Contributor
There was a problem hiding this comment.
The ForAny backport mirrors ForAll's structure, and the walk.go refactor to walkAndMutate looks right.
Verified at 891c1b80 (Go 1.27.3):
go test -race -count=3 ./wait/passes andgo vet ./wait/is clean.- A throwaway test that removed the middle
ForLogfromForAny(nop, log, nop)throughWalkreturned no error and left 2 strategies, so the newwalkbranch works. mockStrategyTarget,anyContextandnilWaitStrategyall exist, and nothing outsidewait/type-switches on*MultiStrategy.
Inline comments cover the ForAll value-type panic, a missing walk test, a no-op for loop, the wall-clock health test and the example's timeouts. None of them blocks the merge.
PR description: the commit table lists 5 commits but the PR has 6. The sixth, fix(wait): support non-pointer Strategy implementations in ForAny, is go-sdk-only and has no Backported-from: trailer. Please add it to the table.
Merge method: .github/settings.yml sets allow_rebase_merge: false, so preserving authorship needs either that setting flipped or a squash with the three Co-authored-by: trailers from the description. That is a maintainer decision.
Reviewed by Claude Code on behalf of @robmry.
Reuse isNilStrategy in MultiStrategy so ForAll(valueStrategy{}) no longer
panics in String and WaitUntilReady, matching ForAny. Adds a regression test.
Addresses review feedback on PR #171.
…meouts - any.go: the select always returns, drop the redundant for loop - walk_test.go: add a Walk case for AnyMultiStrategy (visit, remove) - wait_examples_test.go: ExampleForAny no longer shows a 10s timeout that would fail the whole ForAny early; note that the shortest timeout bounds it Addresses review feedback on PR #171.
robmry
approved these changes
Oct 23, 2026
mdelapenya
added a commit
to testcontainers/testcontainers-go
that referenced
this pull request
Oct 23, 2026
…#4052) reflect.Value.IsNil panics for non-nil-capable kinds such as structs, so ForAny(valueStrategy{}) and ForAll(valueStrategy{}) crashed in String and WaitUntilReady. Add an isNilStrategy helper that only calls IsNil on nil-capable kinds and use it in AnyMultiStrategy and MultiStrategy. Also tidy AnyMultiStrategy.WaitUntilReady (the select always returns, so drop the for loop), cover Walk on ForAny, and fix ExampleForAny timeouts: a strategy hitting its own timeout fails the whole ForAny, so the shortest timeout bounds the wait. Backport of fixes found in docker/go-sdk#171. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backports the missing
wait/changes from testcontainers-go intocontainer/wait, preserving original authorship (each commit keeps its upstream author and date, aBackported-from:trailer with the upstream SHA, and "Backport notes" listing the adaptations). Added withgit format-patch -1 <sha> -- wait/ | git am -3 --directory=container.Refs #147 (already closed; this delivers
wait.ForAny).Commits
slices.Deleteinwalk.go; keeps go-sdk'sErrVisitStop// +buildline inerrors.gowait.ForAny/AnyMultiStrategy. Adapted:ErrVisitStop,&NopStrategyTarget{},WithTimeout, typednetwork.Port, import path. Thesynctesttest stays inany_test.gowith no//go:build go1.25tag (go-sdk is on Go 1.27). Upstream issue: testcontainers/testcontainers-go#3717WithTimeout; dropped strayLogger()hunkwait.ForAnyincontainer/README.mdBackported-from:)fix(wait): support non-pointer Strategy implementations in ForAny— review follow-up for areflect.IsNilpanic in the upstream codeBackported-from:)fix(wait): support non-pointer Strategy implementations in ForAll— same fix forMultiStrategyBackported-from:)ForAnyselect, add aWalktest forAnyMultiStrategy, fixExampleForAnytimeoutsCommits 1–4 are the upstream-authored backports; 5–8 are go-sdk-authored.
Not included
network.Port.Port()frommoby/moby/api>= v1.55.0 (go-sdk pins v1.52.0).net.Dialer.DualStackremoval andlog_test.goparen fix are already inmainvia chore: bump Go to 1.27 (toolchain go1.27.3) #169, so the pick was empty and skipped.ExecStrategyPollInterval cap /WithRetryOnError) are untouched.Merge method — maintainer's choice
Authorship is preserved only with "Rebase and merge".
.github/settings.ymlcurrently hasallow_rebase_merge: false(I did not modify it). A squash merge would attribute everything to the PR author; in that case add these trailers to the squash message:Verification
After every commit:
go build ./...,go vet ./wait/,go test ./wait/(Go 1.27.3) and golangci-lint v2.15.0 oncontainer/wait— all clean.