chore(wait): replace manual StrategyTarget mocks with generated mockery mocks#3603
chore(wait): replace manual StrategyTarget mocks with generated mockery mocks#3603mateenali66 wants to merge 2 commits into
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
Summary by CodeRabbit
This release includes internal test improvements with no user-facing changes. WalkthroughTwo wait-strategy test files ( ChangesWait Strategy Test Migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
wait/exec_test.go (1)
69-76: Consider usingRunAndReturn()for clearer intent with dynamic return values.The current
Return()pattern with a function argument works correctly because mockery-generated mocks have special handling that checks if the return value is a function matching the method signature and executes it. However,RunAndReturn()is more explicit and idiomatic for this use case:target.On("Exec", mock.Anything, mock.Anything, mock.Anything).RunAndReturn( func(ctx context.Context, cmd []string, opts ...tcexec.ProcessOption) (int, io.Reader, error) { if time.Now().After(successAfter) { return 0, nil, nil } return 10, nil, nil }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wait/exec_test.go` around lines 69 - 76, Replace the current mock setup that uses Return with a clearer RunAndReturn call: on the mock `target` where you call On("Exec", mock.Anything, mock.Anything, mock.Anything) switch from .Return(func(ctx context.Context, cmd []string, opts ...tcexec.ProcessOption) (int, io.Reader, error) { ... }) to .RunAndReturn(func(ctx context.Context, cmd []string, opts ...tcexec.ProcessOption) (int, io.Reader, error) { ... }); keep the same function signature and the dynamic successAfter logic so the mock executes that function at call time instead of relying on Return's special-case behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wait/exec_test.go`:
- Around line 88-96: The mock setup for target.On("Exec", ...) incorrectly uses
Return() with a function literal so the function isn't executed for each call;
change this to use RunAndReturn() on the mock so the provided func(ctx
context.Context, cmd []string, opts ...tcexec.ProcessOption) (int, io.Reader,
error) is invoked at call time, preserve the logic that sleeps, checks ctx.Err()
and returns accordingly, and keep the same argument matchers (mock.Anything,
mock.Anything, mock.Anything) and return-value types to match the Exec method
signature.
In `@wait/health_test.go`:
- Around line 52-60: The mock currently uses Return(...) with a function literal
which will be returned as the value instead of being invoked; replace the
Return(...) call on the target mock for the "State" method with
RunAndReturn(...) so the provided function is executed each call. Specifically,
change target.On("State", mock.Anything).Return(func(_ context.Context)
(*container.State, error) { ... }) to target.On("State",
mock.Anything).RunAndReturn(func(ctx context.Context) (*container.State, error)
{ mtx.Lock(); defer mtx.Unlock(); s := *state; return &s, nil }) (i.e., provide
a function matching the State signature so the mock invokes it dynamically and
still returns a copy to avoid races).
---
Nitpick comments:
In `@wait/exec_test.go`:
- Around line 69-76: Replace the current mock setup that uses Return with a
clearer RunAndReturn call: on the mock `target` where you call On("Exec",
mock.Anything, mock.Anything, mock.Anything) switch from .Return(func(ctx
context.Context, cmd []string, opts ...tcexec.ProcessOption) (int, io.Reader,
error) { ... }) to .RunAndReturn(func(ctx context.Context, cmd []string, opts
...tcexec.ProcessOption) (int, io.Reader, error) { ... }); keep the same
function signature and the dynamic successAfter logic so the mock executes that
function at call time instead of relying on Return's special-case behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d29c677-cdf8-4ba1-9558-0d9d20e64ca1
📒 Files selected for processing (3)
wait/exec_test.gowait/exit_test.gowait/health_test.go
mateenali66
changed the title
|
I sent you my review changes in mateenali66@39010f0 🙏 |
- 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#3603.
Satisfies the thelper linter; newStateTarget is a test helper. Signed-off-by: Mateen Anjum <mateenali66@gmail.com>
mateenali66
force-pushed
the
feat/issue-2765-generated-mocks-wait
branch
from
728129a to
77c1ac7
Compare
|
thanks @mdelapenya. applied your commit on top of current main (the branch was conflicting against the moby/moby migration that landed since March), and added t.Helper() to newStateTarget so it passes the thelper linter. wait tests pass and golangci-lint v2.9.0 is clean on ./wait/... you can close your suggestions PR, the changes are in here now. |
There was a problem hiding this comment.
Pull request overview
This PR updates the wait package tests to stop using hand-written StrategyTarget test doubles and instead rely on the generated mockStrategyTarget (mockery-based), aligning these tests with the external wait_test package pattern used elsewhere in the folder.
Changes:
- Switched
health_test.goandexit_test.gotopackage wait_testand updated imports accordingly. - Replaced manual
StrategyTargetmock structs with the generatedmockStrategyTargetand added a small helper (newStateTarget) for commonState()setups.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| wait/health_test.go | Migrates health strategy tests to wait_test and uses generated mockStrategyTarget (plus a helper) instead of a manual mock. |
| wait/exit_test.go | Migrates exit strategy test to wait_test and uses generated mockStrategyTarget for State() behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| package wait_test | ||
|
|
||
| import ( |
Part of #2765. Replaces the manual StrategyTarget mock structs in exec_test.go, exit_test.go, and health_test.go with the generated mockStrategyTarget from strategytarget_mock_test.go. Tests moved to package wait_test for consistency with the generated mock.