Skip to content

test: correct the REST deviation records - #725

Open
owenpearson wants to merge 1 commit into
mainfrom
uts/deviations-corrections
Open

owenpearson wants to merge 1 commit into
mainfrom
uts/deviations-corrections

Conversation

@owenpearson

@owenpearson owenpearson commented Sep 30, 2026 •

Copy link
Copy Markdown
Member

A triage of the REST entries in test/uts/deviations.md found several that are wrong: an SDK gap that isn't one, spec errors filed as deviations, a spec fault blamed on the wrong spec, adapted tests listed as failing, wrong labels, and statuses that don't hold up against features.md. This corrects them and re-measures the counts in the header.

Test changes

Tests change only where their classification changes.

  • TP3a/d/g are un-gated. The SDK already fills these fields from the ProtocolMessage: RealtimeChannel._on_message calls Message.update_inner_message_fields before decoding. The derived helper skipped that step. It now makes the same call, and TP3g compares against a datetime, as test_tp3_presence_from_json already does. All three pass.
  • RSA16c (expiry renewal) and RSA16d (switch to basic) move from @deviation to @spec_error.

deviations.md corrections

  • Batch envelopes: the spec-error entry had the wrong spec at fault. RSC22b has batchPublish return "an array of BatchResults". ably-js's sandbox tests and the sandbox's own GET /presence?channels= response both use the envelope, so the fault is batch_publish.md's flat results, not batch_presence.md. This was never filed upstream.
  • Moved: RSA4 and RSA12a were listed under Failing Tests, but their tests are adapted, not gated.
  • Labels:
    • The httpRequestTimeout row is TO3l4, not TO3l1 (disconnectedRetryTimeout).
    • The TI row is TI1/TI4.
  • Statuses:
    • RSC15a is an off-by-one: TO3l5 counts fallback hosts, so the default allows 4 attempts, as ably-js, ably-java and ably-go make.
    • REC1b1/c1 is compliant with RSC1b's 40106.
    • RSA6b/d: canonicalisation is permitted, not required by RSA9f.
    • RSC19e: @catch_all would not close it.
    • HP8: its prose and IDL disagree, and it has a non-breaking fix.
  • Claims:
    • validate_message_size is not a TM6 calculation.
    • RSA16b's invented expiry cannot drive renewal for a token-string client.
    • RSA10b and RSA8c1a also need the RSA12a fix.
    • RSA15a's three tests cannot all pass as written.
    • A lowercase authorization header sends two auth headers.
    • urljoin resolves dot-segments in device ids.
    • RSL2's quote_plus reaches presence, annotations, serials and realtime history too.
    • Presence already sends %3A.
    • The logging tests need log events the SDK never emits.
    • The test_ti_errorinfo_from_json adaptation is now recorded.
    • Stale line references for the realtime size check are updated.
  • New spec faults, marked not yet filed:
    • The RSL1i at-limit fixture: 5 + 1024 > 1024 under TM6a.
    • client_id.md: RSA15a timing, and the RSA12a/b labels.
    • token_request_params.md: RSA5c/RSA6c labels.
    • request.md: RSC19b "may vary".
    • fallback.md: REC1b1/c1 40000.
    • batch_publish.md: RSC22_Error1/2.
    • features.md: HP8 prose vs IDL.
  • Also recorded:
    • The test_to3_client_options_custom_hosts adaptation.
    • fallbackHostsUseDefault's REC2b test, and its removal in 2.0.
  • Housekeeping: three Smaller faults rows appeared twice.

Counts

Measured by collecting the suite and mapping each item to its # UTS: id. Against unchanged main, this reproduces the existing header exactly.

  • Test IDs: 907 pass, 210 are gated (215 cases) and 15 cannot run. Spec faults go from 10 to 12 Test IDs, which reduce to 8 root causes. SDK root causes go from 71 to 68, 24 of them REST.
  • helpers/ cases: 130, not 122.
  • Closing counts section: it had drifted from the header, and now matches the run.

Not in this PR

Testing

  • uv run --frozen --extra crypto --extra dev pytest test/uts -q: 1134 passed, 230 skipped.
  • RUN_DEVIATIONS=1 uv run --frozen --extra crypto --extra dev pytest test/uts -q: 215 failed, 1134 passed, 15 skipped. Every gated case fails when enabled.
  • uv run ruff check: clean.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Documentation
    • Updated compatibility notes with current test status, specification gaps, REST behavior observations, and adoption counts.
    • Clarified reported issues involving authentication, message handling, logging, and REST requests.
  • Tests
    • Updated conformance expectations for authentication renewal, authentication mode changes, and presence-message timestamps.
    • Corrected presence-message test setup to include protocol-message attributes.

A triage of the REST entries in deviations.md found several that do not
hold against features.md or the code.

TP3a/d/g are not an SDK gap: RealtimeChannel._on_message fills the
presence fields through Message.update_inner_message_fields, and the
derived helper now does the same, so the three tests pass ungated.
RSA16c's expiry renewal and RSA16d's switch to basic auth are spec
errors (RSA4b1, and RSA10a/e/f), so they are marked @spec_error.

The batch envelope entry now names batch_publish.md as the spec at
fault, since RSC22b returns an array of BatchResults. RSA4 and RSA12a
move to the adapted rows they belong in, labels and statuses are
corrected, new spec faults are recorded as not yet filed, duplicate
rows are removed, and the header counts are re-measured.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: c51e8250-2d10-4ca5-80c9-52b939f025b3

📥 Commits

Reviewing files that changed from the base of the PR and between 37bfe83 and e4dd980.

📒 Files selected for processing (3)
  • test/uts/deviations.md
  • test/uts/rest/unit/auth/token_details_test.py
  • test/uts/rest/unit/types/presence_message_types_test.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

The deviation document updates UTS counts, specification-fault entries, and REST behavior observations. REST unit tests update token-detail deviation classifications and presence-message decoding expectations.

Changes

UTS deviation records and REST tests

Layer / File(s) Summary
Specification-fault catalog
test/uts/deviations.md
The document updates deviation counts and filing notes. It adds RSA16 specification-error entries and revises related specification-point, fixture, and batch-response records.
REST behavior and feature inventory
test/uts/deviations.md
The document expands notes on REST authentication, request behavior, response shapes, message-size checks, and fallback-host behavior.
Test classification and decoding
test/uts/rest/unit/auth/token_details_test.py, test/uts/rest/unit/types/presence_message_types_test.py, test/uts/deviations.md
Two token-detail tests change from @deviation to @spec_error. Presence-message tests populate inner attributes before decoding and assert the timestamp as a datetime. The document updates test-run counts and failing-test notes.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Merge Risk: ⚪ Minimal · up to e4dd9

The changes align conformance records and test expectations without changing SDK runtime behavior. No merge-blocking issue remains identified; merge after normal checks pass.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: correcting REST deviation records and related test classifications.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

A rabbit checks the test list twice,
Then marks two cases with spec-error spice.
It nudges presence fields in place,
And stamps the time with date-time grace.
The counts are neat; it hops away.

Comment @coderabbitai help to get the list of available commands.

This branch was successfully deployed

1 active deployment
staging/pull/725/features — e4dd9807 Deployed Sep 30, 2026 by github-actions[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant