Expose timeout on Kotlin client builder; disable in tests#317
Conversation
There was a problem hiding this comment.
Pull request overview
This PR exposes a configurable request timeout on the Kotlin BasecampClientBuilder, makes HttpTimeout opt-in by skipping plugin installation when the configured timeout is Duration.INFINITE, and updates Kotlin tests to construct clients with timeouts disabled by default to avoid runTest virtual-time induced timeouts after the Ktor 3.5.0 bump.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Changes:
- Add
timeout: DurationtoBasecampClientBuilderand pass it through toBasecampConfig. - Only install Ktor’s
HttpTimeoutplugin when the configured timeout is finite. - Introduce
testBasecampClient { ... }(defaults toDuration.INFINITE) and migrate tests to use it.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/BasecampClient.kt | Exposes timeout on the builder and conditionally installs HttpTimeout only for finite timeouts. |
| kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/TestBasecampClient.kt | Adds a shared test helper that disables timeouts by default (using Duration.INFINITE). |
| kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/AuthStrategyTest.kt | Switches test client construction to testBasecampClient. |
| kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/ClientTest.kt | Switches test client construction to testBasecampClient. |
| kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/CommentsServiceTest.kt | Switches test client construction to testBasecampClient. |
| kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/DownloadTest.kt | Switches test client construction to testBasecampClient. |
| kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/PaginationTest.kt | Switches test client construction to testBasecampClient. |
| kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/PeopleServiceTest.kt | Switches test client construction to testBasecampClient. |
| kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/ProjectsServiceTest.kt | Switches test client construction to testBasecampClient. |
| kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/RetryTest.kt | Switches test client construction to testBasecampClient. |
| kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/TodosServiceTest.kt | Switches test client construction to testBasecampClient. |
| kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/WebhooksServiceTest.kt | Switches test client construction to testBasecampClient. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
2 issues found across 12 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
BasecampClientBuilder didn't surface the timeout knob, so callers were stuck on the 30-second default baked into BasecampConfig. Wire it through (with BasecampConfig.DEFAULT_TIMEOUT as the single source of truth), reject non-positive timeouts at build(), and skip installing Ktor's HttpTimeout plugin when the timeout is Duration.INFINITE. In tests, route all client construction through a new testBasecampClient helper that defaults timeout to INFINITE. Ktor 3.5.0 (KTOR-8271) made HttpTimeout honor the kotlinx-coroutines test scheduler's virtual clock, so under runTest the 30s timeout was firing before MockEngine's IO-dispatched response could return — breaking 66 tests. Tests that want to exercise timeout behavior can override the default inside the block. Adds ClientTest cases covering timeout propagation, non-positive rejection, and the INFINITE-skips-install regression guard.
b608f6a to
b777b8e
Compare
Review carefully before merging. Consider a major version bump. |
Summary
BasecampClientBuilderdidn't surface atimeoutknob, so callers were stuck on the 30-second default baked intoBasecampConfig. Wired it through.configureClient()skips installing Ktor'sHttpTimeoutplugin whentimeoutisDuration.INFINITE.testBasecampClient { … }helper that defaultstimeouttoDuration.INFINITE. Tests that want to exercise timeout behavior can override inside the block.Why
Ktor 3.5.0 (KTOR-8271, bumped in #311) made
HttpTimeouthonor the kotlinx-coroutines test scheduler's virtual clock. UnderrunTest, the dispatcher idles whileMockEngineposts its response back fromDispatchers.IO; virtual time auto-advances past 30s; the timeout coroutine fires and every request fails withHttpRequestTimeoutException. 66 tests broke from a single dependency bump.Rather than pinning ktor or sniffing for
MockEnginein production code, this surfacestimeoutas a real public builder field and uses the existing API (set it toINFINITE) to opt out in tests. Production code path is unchanged.Test plan
make kt-testgreen (was 66 failures before this change).timeout = 100.millisecondsinsidetestBasecampClient { … }and assertHttpRequestTimeoutException— a capability KTOR-8271 unlocked.Summary by cubic
Expose a request timeout on the Kotlin
BasecampClientBuilderand disable it by default in tests to prevent false timeouts after the Ktor 3.5.0 update. Centralizes the 30s default, validates the value, and installsHttpTimeoutonly when the timeout is finite.New Features
timeout: DurationtoBasecampClientBuilder(defaults toBasecampConfig.DEFAULT_TIMEOUT= 30s). SetDuration.INFINITEto disable.Duration.INFINITE.configureClient()installsHttpTimeoutonly when the timeout is finite; default moved toBasecampConfig.DEFAULT_TIMEOUTshared by builder and config.Bug Fixes
testBasecampClient { ... }withtimeout = Duration.INFINITEto avoidHttpRequestTimeoutExceptionunderrunTestwith Ktor 3.5.0’s virtual clock. Override the timeout in tests when needed.Written for commit b777b8e. Summary will update on new commits. Review in cubic