fix: apply withStartupTimeout() to the port-binding pre-wait - #1447
fix: apply withStartupTimeout() to the port-binding pre-wait#1447kenzox wants to merge 1 commit into
Conversation
`GenericContainer.start()` waits for host port bindings via `inspectContainerUntilPortsExposed` before the wait strategy runs, but neither call site passed `this.startupTimeoutMs`, so that pre-wait was always capped at the util's 10 s default and `withStartupTimeout()` did not apply to it. Pass the configured value at both call sites; when no startup timeout was configured the argument is `undefined` and the 10 s default still applies. Fixes testcontainers#1446 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
✅ Deploy Preview for testcontainers-node 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 Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. Summary by CodeRabbit
Walkthrough
ChangesContainer startup timeout
Estimated code review effort: 2 (Simple) | ~5 minutes Merge Risk: ⚪ Minimal · up to The configured startup timeout now also covers port-binding waits while the existing default behavior remains unchanged; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue [ Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
ESLint install failed: one or more packages not found in the registry. 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 |
![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: b917d69b3c
ℹ️ 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".
| () => client.container.inspect(container), | ||
| container.id | ||
| container.id, | ||
| this.startupTimeoutMs |
There was a problem hiding this comment.
This bug fix changes GenericContainer.start() behavior, but there is no test proving that a custom withStartupTimeout() lets the port-binding pre-wait run past the utility's 10s default; a future refactor could silently reintroduce the cap. Please add a focused regression test around this startup path that fails before the change and passes after it.
AGENTS.md reference: AGENTS.md:L29-L34
Useful? React with 👍 / 👎.
1 participant
Fixes #1446.
GenericContainer.start()waits for host port bindings (inspectContainerUntilPortsExposed) before the wait strategy runs, but neither call site passesthis.startupTimeoutMs, so that pre-wait is always capped at the util's 10 s default andwithStartupTimeout()does not apply to it.This passes
this.startupTimeoutMsat both call sites (reuseContainerandstartContainer). When no startup timeout was configured the argument isundefinedand the existing 10 s default still applies, so behaviour only changes for callers who explicitly asked for a longer budget.Measured (12.1.0, six
postgres:18-alpinecontainers starting concurrently on a 2-vCPU GitHub runner): port binding took up to 16.7 s and failed at 10 s despitewithStartupTimeout(120_000); with this change the same suite passes.Verified locally on the branch:
npm ci→ 0,tsc -p packages/testcontainers/tsconfig.json --noEmit→ 0, eslint → 0, prettier → 0. The Docker-backed integration suite was not run here.