-
Notifications
You must be signed in to change notification settings - Fork 32
Add portable unit test specs for ably-go REST client #694
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
ff88508
f49143b
ea58cf2
bbc4b8d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,266 @@ | ||
| # Spec Test Anomalies | ||
|
|
||
| Issues discovered while implementing portable test specs from `specification/md/test/specs/`. | ||
|
|
||
| Related: [specification PR #421](https://github.com/ably/specification/pull/421) - Portable unit test specifications | ||
|
|
||
| ## Test Results Summary (Latest Run) | ||
|
|
||
| ``` | ||
| go test -v -tags=!integration ./ably/... 2>&1 | grep -E "(PASS|FAIL|SKIP|---)" | ||
| ``` | ||
|
|
||
| **Total spec tests:** 69 | ||
| - **PASS:** 63 | ||
| - **SKIP:** 6 (with documented deviations) | ||
| - **FAIL:** 0 | ||
|
|
||
| **Skipped tests (require investigation):** | ||
| 1. `TestTokenRenewal_RSA4b4_RenewalOnExpiryRejection` - ably-go REST client doesn't auto-retry | ||
| 2. `TestTokenRenewal_RSA4b4_RenewalOn40140Error` - ably-go REST client doesn't auto-retry | ||
| 3. `TestTokenRenewal_RSA4b4_RenewalWithAuthUrl` - ably-go REST client doesn't auto-retry | ||
| 4. `TestTokenRenewal_RSA4b4_RenewalLimit` - ably-go REST client doesn't auto-retry | ||
| 5. `TestAuthScheme_RSC18_UsingJWT` - JWT support needs investigation | ||
| 6. `TestAuthCallback_RSA8d_JwtTokenReturned` - JWT support needs investigation | ||
|
|
||
| Pre-existing tests that fail due to missing fixtures (not spec tests): | ||
| - `Test_decodeMessage` - missing `../common/test-resources/messages-encoding.json` | ||
| - `TestMsgpackDecoding` - missing `../common/test-resources/msgpack_test_fixtures.json` | ||
| - `TestMessage_CryptoDataFixtures_*` - missing crypto test fixtures | ||
|
|
||
| --- | ||
|
|
||
| ## Deviations from Spec | ||
|
|
||
| ### 1. Token Base64 Encoding (RSA3, RSA4a, RSA4) | ||
|
|
||
| **Issue:** The spec expects Bearer tokens to be sent raw, but ably-go base64 encodes them. | ||
|
|
||
| | Expected (per spec) | Actual (ably-go) | | ||
| |---------------------|------------------| | ||
| | `Bearer my-token-string` | `Bearer bXktdG9rZW4tc3RyaW5n` | | ||
|
Comment on lines
+39
to
+41
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Align table columns to satisfy markdownlint MD060. The tables are valid Markdown, but MD060 expects aligned pipes. 📝 Suggested fix-| Expected (per spec) | Actual (ably-go) |
-|---------------------|------------------|
-| `Bearer my-token-string` | `Bearer bXktdG9rZW4tc3RyaW5n` |
+| Expected (per spec) | Actual (ably-go) |
+|--------------------------|--------------------------------|
+| `Bearer my-token-string` | `Bearer bXktdG9rZW4tc3RyaW5n` |
@@
-| Spec expects | ably-go uses |
-|--------------|--------------|
-| `main.realtime.ably.net` | `rest.ably.io` |
-| `sandbox.realtime.ably.net` | `sandbox-rest.ably.io` |
+| Spec expects | ably-go uses |
+|-----------------------------|-----------------------|
+| `main.realtime.ably.net` | `rest.ably.io` |
+| `sandbox.realtime.ably.net` | `sandbox-rest.ably.io` |Also applies to: 85-88 🧰 Tools🪛 markdownlint-cli2 (0.20.0)[warning] 41-41: Table column style (MD060, table-column-style) [warning] 41-41: Table column style (MD060, table-column-style) 🤖 Prompt for AI Agents |
||
|
|
||
| **Question:** Is this intentional behavior or a bug? Need to verify against other SDK implementations. | ||
|
|
||
| --- | ||
|
|
||
| ### 2. RSA4b - key + clientId Does Not Auto-Trigger Token Auth | ||
|
|
||
| **Issue:** The spec states that providing `key + clientId` should automatically trigger token auth. However, ably-go requires explicit `WithUseTokenAuth(true)`. | ||
|
|
||
| **ably-go behavior:** Uses Basic auth unless `WithUseTokenAuth(true)` is also set. | ||
|
|
||
| --- | ||
|
|
||
| ### 2a. RSA4b4 - Token Renewal Auto-Retry Not Implemented (REST) | ||
|
|
||
| **Issue:** The spec (RSA4b4) states that when a request fails with a token error (40140-40149 range), the library should automatically obtain a new token and retry the request. However, ably-go's REST client does **not** implement this automatic retry behavior. | ||
|
|
||
| **Expected behavior (per spec):** | ||
| 1. Request fails with 40142 (Token expired) | ||
| 2. Library automatically renews token via authCallback/authUrl | ||
| 3. Library retries the original request with new token | ||
| 4. Returns success to caller | ||
|
|
||
| **ably-go REST behavior:** | ||
| 1. Request fails with 40142 (Token expired) | ||
| 2. Error is returned directly to caller (no automatic retry) | ||
|
|
||
| **Skipped tests:** | ||
| - `TestTokenRenewal_RSA4b4_RenewalOnExpiryRejection` | ||
| - `TestTokenRenewal_RSA4b4_RenewalOn40140Error` | ||
| - `TestTokenRenewal_RSA4b4_RenewalWithAuthUrl` | ||
| - `TestTokenRenewal_RSA4b4_RenewalLimit` | ||
|
|
||
| **Note:** Pre-emptive renewal (RSA14) - detecting an expired token *before* making a request - appears to work correctly. It's only the reactive retry-on-401 that's not implemented. | ||
|
|
||
| **Investigation needed:** Is this behavior intentional (REST vs Realtime difference)? Do other SDKs implement auto-retry for REST? | ||
|
|
||
| --- | ||
|
|
||
| ### 3. Host Naming Convention | ||
|
|
||
| **Issue:** ably-go uses different host naming than the spec expects. | ||
|
|
||
| | Spec expects | ably-go uses | | ||
| |--------------|--------------| | ||
| | `main.realtime.ably.net` | `rest.ably.io` | | ||
| | `sandbox.realtime.ably.net` | `sandbox-rest.ably.io` | | ||
|
|
||
| **Skipped tests:** | ||
| - `TestFallback_REC1_DefaultEndpoint` | ||
| - `TestFallback_REC1_SandboxEndpoint` | ||
| - `TestFallback_RSC15a_FallbackHostsTried` | ||
| - `TestOptionsTypes_TO_EndpointHostSelection` | ||
|
|
||
| --- | ||
|
|
||
| ### 4. ClientId Propagation Timing | ||
|
|
||
| **Issue:** ably-go doesn't immediately populate `Auth.ClientID()` from TokenDetails provided at construction time. The clientId is only available after first authenticated request. | ||
|
|
||
| **Skipped tests:** | ||
| - `TestClientId_RSA7b_FromTokenDetails` | ||
| - `TestClientId_RSA7_InheritFromToken` | ||
| - `TestClientId_RSA7_UpdatedAfterAuthorize` | ||
|
|
||
| --- | ||
|
|
||
| ### 5. Message Encoding/Decoding | ||
|
|
||
| **Issue:** ably-go's message encoding/decoding behavior differs from spec in several ways: | ||
| - Base64 decoding may return string instead of `[]byte` | ||
| - JSON decoding may not produce `map[string]interface{}` as expected | ||
| - Chained encoding handling differs | ||
| - Unrecognized encoding handling differs | ||
|
|
||
| **Skipped tests:** | ||
| - `TestMessageEncoding_RSL6a_DecodingBase64` | ||
| - `TestMessageEncoding_RSL6a_DecodingJSON` | ||
| - `TestMessageEncoding_RSL6a_DecodingChainedEncodings` | ||
| - `TestMessageEncoding_RSL6b_UnrecognizedEncoding` | ||
|
|
||
| --- | ||
|
|
||
| ### 6. TokenParams Missing Nonce Field (TK5) | ||
|
|
||
| **Issue:** The spec says `TokenParams` should have a `nonce` field, but ably-go only has `Nonce` in `TokenRequest`, not `TokenParams`. | ||
|
|
||
| --- | ||
|
|
||
| ### 6a. Error Code Differences (RSA4b) | ||
|
|
||
| **Issue:** The spec expects error code 40106 (No authentication credentials) when no credentials are provided. ably-go returns different codes depending on context: | ||
| - `40005` - when no key or token provided | ||
| - `40101` - in some authentication error scenarios | ||
|
|
||
| **Workaround:** Tests accept multiple error codes: `40005 || 40101 || 40106` | ||
|
|
||
| --- | ||
|
|
||
| ### 6b. JWT Token Support (RSA8d, RSC18) | ||
|
|
||
| **Issue:** The spec defines JWT token support where: | ||
| - `authCallback` can return a JWT string directly (RSA8d) | ||
| - The library should detect JWT format and use it appropriately (RSC18) | ||
|
|
||
| **ably-go behavior:** JWT support needs investigation. The tests are skipped pending verification of how ably-go handles JWT tokens returned from authCallback. | ||
|
|
||
| **Skipped tests:** | ||
| - `TestAuthScheme_RSC18_UsingJWT` | ||
| - `TestAuthCallback_RSA8d_JwtTokenReturned` | ||
|
|
||
| --- | ||
|
|
||
| ### 7. ably-go Authorize() Nil Pointer Bug | ||
|
|
||
| **Issue:** Calling `client.Auth.Authorize(ctx, nil, authOption...)` causes a nil pointer dereference when `authOption` is provided but `params` is nil. | ||
|
|
||
| **Location:** `auth.go:358` - tries to set `params.Timestamp = 0` when `params` is nil | ||
|
|
||
| **Workaround:** Always pass `&ably.TokenParams{}` instead of `nil` | ||
|
|
||
| --- | ||
|
|
||
| ### 8. ably-go Uses Msgpack by Default | ||
|
|
||
| **Issue:** Tests assume JSON encoding for request bodies, but ably-go uses msgpack by default. Tests use `ably.WithUseBinaryProtocol(false)` to force JSON for easier test inspection. | ||
|
|
||
| --- | ||
|
|
||
| ### 9. Fallback Behavior Differences | ||
|
|
||
| **Issue:** ably-go's fallback and retry behavior differs from spec: | ||
| - Empty fallback hosts may still trigger retries | ||
| - Cached fallback expiration timing differs | ||
| - Custom endpoint handling differs | ||
|
|
||
| **Skipped tests:** | ||
| - `TestFallback_RSC15m_NoFallbackWhenEmpty` | ||
| - `TestFallback_RSC15f_CachedFallbackExpires` | ||
| - `TestFallback_REC2_CustomHostNoFallback` | ||
| - `TestFallback_REC2_CustomFallbackHosts` | ||
|
|
||
| --- | ||
|
|
||
| ### 10. Pagination Behavior | ||
|
|
||
| **Issue:** ably-go's Pages iterator behavior differs from spec: | ||
| - No `first()` method | ||
| - `Items()` behavior after `Next()` returns false differs | ||
|
|
||
| **Skipped tests:** | ||
| - `TestPaginatedResult_TG4_FirstPage` | ||
| - `TestPaginatedResult_TG_NextOnLastPage` | ||
|
|
||
| --- | ||
|
|
||
| ### 11. URL Encoding in Channel Names | ||
|
|
||
| **Issue:** The spec expects URL-encoded channel names (e.g., `with%3Acolon`), but ably-go does NOT URL-encode special characters in channel name paths. | ||
|
|
||
| **Test behavior:** Tests updated to expect ably-go's actual behavior with DEVIATION comments. | ||
|
|
||
| --- | ||
|
|
||
| ### 12. UseTokenAuth Without Credentials | ||
|
|
||
| **Issue:** ably-go allows `WithUseTokenAuth(true)` without other credentials, deferring the error to first request. Spec expects immediate error. | ||
|
|
||
| **Skipped test case:** | ||
| - `TestClientOptions_RSC1b_InvalidArgumentsError/useTokenAuth_only` | ||
|
|
||
| --- | ||
|
|
||
| ## Action Items | ||
|
|
||
| ### High Priority (Blocking spec compliance) | ||
| - [ ] **RSA4b4:** Investigate whether REST client should auto-retry on token expiry (40140-40149 errors) | ||
| - [ ] **RSA8d/RSC18:** Investigate JWT token support in ably-go | ||
| - [ ] **RSA4b:** Clarify whether token base64 encoding in Bearer header is intentional | ||
| - [ ] **RSA4b:** Determine if `key + clientId` should auto-trigger token auth | ||
|
|
||
| ### Medium Priority | ||
| - [ ] **RSA4b:** Investigate error code differences (40005 vs 40106 vs 40101) | ||
| - [ ] Report Authorize() nil pointer bug to ably-go maintainers (line 358) | ||
| - [ ] Investigate clientId propagation timing (RSA7b) | ||
| - [ ] Investigate message encoding/decoding differences (RSL6) | ||
|
|
||
| ### Low Priority | ||
| - [ ] Investigate host naming convention differences (rest.ably.io vs realtime.ably.net) | ||
| - [ ] Investigate fallback behavior differences (RSC15) | ||
| - [ ] Investigate pagination behavior differences (TG4) | ||
|
|
||
| --- | ||
|
|
||
| ## Test Files Created | ||
|
|
||
| ### Authentication Tests | ||
| - `auth_scheme_spec_test.go` - RSA1-4, RSA4b, RSC18 auth scheme selection tests | ||
| - `auth_callback_spec_test.go` - RSA8c, RSA8d auth callback/authUrl tests (incl. JWT) | ||
| - `authorize_spec_test.go` - RSA10 authorize tests | ||
| - `token_renewal_spec_test.go` - RSA4b4, RSA14 token renewal and expiry tests | ||
|
|
||
| ### Client Tests | ||
| - `client_id_spec_test.go` - RSA7, RSA12 clientId tests | ||
| - `client_options_spec_test.go` - RSC1, TO3 client options tests | ||
| - `rest_client_spec_test.go` - REST client initialization tests | ||
| - `time_spec_test.go` - RSC16 server time tests | ||
| - `stats_spec_test.go` - RSC6 application statistics tests | ||
|
|
||
| ### Channel Tests | ||
| - `channel_publish_spec_test.go` - RSL1 channel publish tests | ||
| - `channel_history_spec_test.go` - RSL2 channel history tests | ||
| - `channel_idempotency_spec_test.go` - RSL1k idempotency tests | ||
|
|
||
| ### Message Tests | ||
| - `message_encoding_spec_test.go` - RSL4, RSL6 message encoding tests | ||
| - `message_types_spec_test.go` - TM2-5 message types tests | ||
|
|
||
| ### Infrastructure Tests | ||
| - `options_types_spec_test.go` - TO3, AO2 options tests | ||
| - `fallback_spec_test.go` - RSC15, REC1-2 fallback tests | ||
| - `paginated_result_spec_test.go` - TG1-4 pagination tests | ||
| - `error_types_spec_test.go` - TI1-2 error types tests | ||
| - `token_types_spec_test.go` - TK token types tests | ||
| - `crypto_spec_test.go` - RSE1-2 crypto tests | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a language identifier to the fenced block.
markdownlint MD040 flags this fence. Adding a language keeps lint clean and improves readability.
📝 Suggested fix
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents