Support running the integration suite against ABLY_LOCAL_SANDBOX_URL - #708
Merged
Conversation
Each case in this test appended WithTLS(true) to the sandbox options. The assertions only concern query-time/timestamp handling in Authorize, so the TLS override is incidental — but because per-test options are merged after the sandbox defaults, it overrode the transport the harness selected. Against a plaintext local server that forced HTTPS on the default TLS port instead of the app's plain port, so the token request stalled and the test timed out. Dropping it lets the test inherit the harness's transport (TLS against the cloud sandbox, as before; plain against a local ably-sandbox), and it passes in both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…wedged dial The test's mock dial blocked on an empty dialErr channel whenever the connection attempted a reconnect the test hadn't pre-fed an outcome for. A dial blocked there wedges the connection in CONNECTING, where the DISCONNECTED->SUSPENDED timer does not run, so the connection never became SUSPENDED and the test hung until the outer timeout — despite exercising a fully mocked transport that never touches the server. Make the mock sticky: reuse the most recently injected dial outcome when the channel is empty instead of blocking, mirroring a real dial (which always returns within its timeout). The connection now retries freely, reaches SUSPENDED on the shortened TTL, and the test passes in ~1s. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
TestRealtimeConn_RTN22a_RTN15h2_/RTN22_RTC8_Integration_ServerInitiatedAuth and the RTC8a4 JWT-reauth subtest each constructed their Realtime client directly with a bare WithEndpoint(ablytest.Endpoint) and no port/TLS override, discarding the Sandbox returned by NewREST. That bypasses the harness's transport, so against a plaintext local server the client dials the default TLS port, never connects, and the test's unbounded Connect wait hangs until the outer timeout — a harness artifact, not a server result (cf. ably-server TASK-78). Build these clients from app.Options(...) like the rest of the suite so they use the provisioned app's endpoint/port/TLS. Against the cloud sandbox this is the same transport as before; against a local ably-sandbox the three tests now connect and exercise the real behaviour — RTN22/RTN22a and RTC8a4 pass, and the one genuine gap they surface (RTC8a's channel detach on capability downgrade) is now a real failure rather than an artefact. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
TestAuth_JWT_Token_RSA8c and TestIdempotent_retry built their secondary request
paths from a bare WithEndpoint(app.Endpoint)/WithEndpoint(ablytest.Endpoint)
with no port/TLS — the JWT-auth REST clients directly, and the idempotent-retry
proxy's target URL via ApplyOptionsWithDefaults(nopts...). Since a per-test
local app's endpoint/port/TLS live only in app.Options(), those requests went to
the endpoint name's default host rather than the provisioned child, which 404s
the app id ("No application found with id ..."). With the cloud sandbox the
endpoint name resolved to the right host so it happened to work.
Build these from app.Options(...) so the JWT clients and the proxy's default URL
use the provisioned app's transport. TestIdempotent_retry now passes; the JWT
test's four connect paths pass, and its remaining failure is a genuine server
divergence (the local server returns 40101 rather than 40144 for an invalid
JWT), not a harness artifact.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ostFallback TestRest_rememberHostFallback (RSC15f) had the same defaultURL-from-nopts bug as TestIdempotent_retry: the client used app.Options() but the proxy's forward target was resolved from nopts alone, so against a per-test local child the "success host" request went to the wrong server and 404'd the app id. Resolve it from app.Options(nopts...) too. The test now passes against the local server. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Across the integration suite, many tests did `x, err := call(); assert.NoError (t, err)` and then immediately dereferenced or indexed x. assert.NoError only records the failure and lets the test continue, so when call() returned (nil, err) — as the client correctly does against an unimplemented or failing server endpoint — execution fell through to a nil dereference or out-of-range index, surfacing as a crash (or a panic-in-teardown hang) rather than a clean FAIL. Convert those sites to require.NoError so the test stops at the failed check. No passing test changes behaviour — require only differs from assert when err is non-nil, where continuing was never valid anyway. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When ABLY_LOCAL_SANDBOX_URL is set, internal/ablytest provisions each test's app through a local sandbox's POST /apps (the same code path as the cloud sandbox) instead of the cloud, and routes clients to the isolated ably-server child it returns (its own endpoint/port/TLS). Every test gets a fresh app, so tests don't contend over shared server state and can run concurrently. The all-powerful test key is selected by parsing each key's capability for the "[*]*" wildcard rather than string-matching it, so provisioning is insensitive to the local sandbox's JSON formatting. The cloud path is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughThe changes add local sandbox routing support, derive integration-test client options from provisioned apps, improve reconnect test dialing, and replace selected non-fatal assertions with fail-fast requirements across authentication, REST, realtime, pagination, and protocol tests. ChangesSandbox routing and integration test updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ably/rest_client_integration_test.go (1)
350-350: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHardcoded cloud sandbox host breaks local sandbox testing.
When running the test suite against a local sandbox (via
ABLY_LOCAL_SANDBOX_URL), the initial requested host (hosts[0]) will be the provisioned local endpoint (e.g.,localhost) rather than the defaultsandbox.realtime.ably-nonprod.net. The hardcoded assertions will cause these tests to fail.Derive the expected host dynamically to support both environments, similar to the fix applied in
TestRest_rememberHostFallback.
ably/rest_client_integration_test.go#L350-L350: Replace the hardcoded host string with a dynamic extraction:expectedPrimaryHost, _ := url.Parse(ably.ApplyOptionsWithDefaults(app.Options(options...)...).RestURL()) assert.Equal(t, expectedPrimaryHost.Hostname(), hosts[0]) // primary hostably/rest_client_integration_test.go#L396-L396: Apply the same dynamic host calculation and assertion here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ably/rest_client_integration_test.go` at line 350, Replace the hardcoded primary-host assertions at ably/rest_client_integration_test.go:350-350 and ably/rest_client_integration_test.go:396-396 with an expected hostname derived from ApplyOptionsWithDefaults(...).RestURL(), then compare its Hostname() to hosts[0] so both local and cloud sandbox environments are supported.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ably/rest_client_integration_test.go`:
- Line 350: Replace the hardcoded primary-host assertions at
ably/rest_client_integration_test.go:350-350 and
ably/rest_client_integration_test.go:396-396 with an expected hostname derived
from ApplyOptionsWithDefaults(...).RestURL(), then compare its Hostname() to
hosts[0] so both local and cloud sandbox environments are supported.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58fedcec-ec93-4b91-9b9e-aaba95743c4f
📒 Files selected for processing (13)
ably/auth_integration_test.goably/http_paginated_response_integration_test.goably/proto_message_integration_test.goably/realtime_channel_integration_test.goably/realtime_channel_spec_integration_test.goably/realtime_client_integration_test.goably/realtime_conn_integration_test.goably/realtime_conn_spec_integration_test.goably/rest_channel_integration_test.goably/rest_channel_spec_integration_test.goably/rest_client_integration_test.gointernal/ablytest/ablytest.gointernal/ablytest/sandbox.go
owenpearson
approved these changes
Jul 22, 2026
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
2 participants
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.
Part of ongoing work to let the SDK test suite run against Ably-compatible open source servers, not just the cloud sandbox.
When
ABLY_LOCAL_SANDBOX_URLis set,internal/ablytestprovisions each test's app through a local sandbox'sPOST /apps(the same code path as the cloud sandbox) instead of the cloud, and routes clients to the isolated server child it returns (its own endpoint/port/TLS). The all-powerful test key is selected by parsing each key's capability for the[*]*wildcard rather than string-matching it, so provisioning is insensitive to the sandbox's JSON formatting.All of this is inert unless
ABLY_LOCAL_SANDBOX_URLis set — the normal cloud-sandbox path is unchanged.🤖 Generated with Claude Code