[AIT-1130] fix(uts): align objects integration tests with corrected specs and run both protocol variants#2256
Conversation
WalkthroughThis PR adds a ChangesProtocol Variant Test Parameterization
RTO20e Proxy Fault Scenario Rewrite
Estimated code review effort: 3 (Moderate) | ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Pull request overview
Updates the UTS LiveObjects integration test suites to match corrected upstream specs and to execute required protocol variants (json + msgpack), improving conformance and coverage for derived objects behavior.
Changes:
- Restructures the proxy faults RTO20e test to exercise the “fail while waiting for SYNCED” path and assert the corrected error/cause.
- Removes/updates prior deviation notes in sync/GC suites to reflect corrected spec wording and clarified tier responsibilities.
- Adds a
describeWithProtocolshelper and applies it to run lifecycle/sync/GC integration suites under both json and msgpack variants.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/uts/objects/integration/proxy/objects_faults.test.ts | Updates RTO20e/RTO20e1 proxy integration scenario and expected error/cause to align with corrected spec flow. |
| test/uts/objects/integration/protocol_variants.ts | Introduces a describe-level helper to run suites for both json and msgpack protocol variants. |
| test/uts/objects/integration/objects_sync.test.ts | Wraps suite in protocol variants and updates RTO4 test title to match corrected spec wording. |
| test/uts/objects/integration/objects_lifecycle.test.ts | Wraps suite in protocol variants; clarifies REST provisioning remains json. |
| test/uts/objects/integration/objects_gc.test.ts | Wraps suite in protocol variants and replaces deviation text with spec-aligned explanations/references. |
Comments suppressed due to low confidence (3)
test/uts/objects/integration/objects_sync.test.ts:32
- setupSandbox/teardownSandbox are defined inside describeWithProtocols, so they will provision and delete a sandbox app once per protocol variant (2× per file). setupSandbox() always provisions a new app, so this adds unnecessary external calls and runtime without improving isolation.
describeWithProtocols('uts/objects/integration/objects_sync', function (useBinaryProtocol) {
this.timeout(30000);
before(async function () {
await setupSandbox();
test/uts/objects/integration/objects_lifecycle.test.ts:32
- setupSandbox/teardownSandbox are currently inside describeWithProtocols, which means a new sandbox app is provisioned and deleted for each protocol variant. Since setupSandbox() always provisions a fresh app, this doubles external provisioning work for the file and can slow/flakify CI.
describeWithProtocols('uts/objects/integration/objects_lifecycle', function (useBinaryProtocol) {
this.timeout(30000);
before(async function () {
await setupSandbox();
test/uts/objects/integration/objects_gc.test.ts:41
- Because setupSandbox/teardownSandbox are defined inside describeWithProtocols, a sandbox app is provisioned and deleted once per protocol variant. setupSandbox() always provisions a new app, so this doubles external provisioning for the file; moving these hooks to root-level avoids the extra app while keeping both protocol variants.
describeWithProtocols('uts/objects/integration/objects_gc', function (useBinaryProtocol) {
this.timeout(30000);
before(async function () {
await setupSandbox();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…son+msgpack variants - objects_faults: restructure the RTO20e proxy test to the corrected spec sequence (ably/specification#501) — force SYNCING via injected ATTACHED with HAS_OBJECTS, mutate while SYNCING, wait for the publish ACK in the proxy log, then inject channel ERROR and assert the pending operation fails with 92008 / statusCode 400 / cause 90000 (RTO20e1). Removes the 90001 DEVIATION, which documented the spec error now fixed at source. - objects_gc: replace the deviation comments with the real spec references — undefined reads are RTLM5d2h-compliant (spec says undefined/null), and the timer-based GC sweep is covered at the unit tier (RTO10/RTO10b1/RTLM19 unit tests), so only observable tombstone semantics belong here. - objects_sync: align the RTO4 subscribe-only test title with the corrected spec wording and drop its DEVIATION comment. - Add describeWithProtocols helper and run objects_lifecycle, objects_sync and objects_gc once per protocol variant (json, msgpack) as their specs require, without any command-line or environment parameter. All suites verified against the nonprod sandbox: objects_faults 5/5, lifecycle/sync/gc 24/24 across both protocol variants.
a1fd9f1 to
346e82a
Compare
Intent
While evaluating the derived objects integration suites against the revised UTS specs, three things surfaced: the RTO20e proxy test asserted the wrong error code because the spec itself was wrong (now fixed at source in ably/specification#501), the GC tests carried deviation comments for behaviour that is actually spec-compliant or already covered at the unit tier, and none of the integration suites honoured their specs'
Protocol Variants: json, msgpackrequirement. This PR aligns the tests with the corrected specs and closes the protocol-variant coverage gap.Changes
1.
proxy/objects_faults.test.ts— RTO20e restructured per the corrected specThe test previously mutated after the channel was already FAILED and asserted 90001 behind a
DEVIATIONcomment. Per RTO26b that path fails the write precondition and never reachespublishAndApply— the spec, not ably-js, was wrong, and ably/specification#501 fixes it. The test now follows the corrected sequence:flags: 128, RTO4c).publishAndApplyparks in the RTO20e sync wait.92008/statusCode 400/cause.code 90000(RTO20e1).The
DEVIATIONcomment is removed and RTO20e1 added to the file's spec points. Test IDs are unchanged.2.
objects_gc.test.ts— deviation comments replaced with spec referencesThe four listed deviations turned out not to be deviations:
objects-features.mdRTLM5d2h says tombstoned entries read back as "undefined/null" —undefinedis the JavaScript mapping, so the assertions are compliant as written.objects/unitRTO10, RTO10b1 and RTLM19 tests verify the actual purge (including the grace-period retention boundary) with controlled clocks, which an integration test on wall-clock time against a real server cannot observe. The header now documents this tier split; test logic is unchanged.3.
objects_sync.test.ts— RTO4 test title alignedThe subscribe-only test's title now matches the corrected spec wording ("Attach without OBJECT_PUBLISH…") and its
DEVIATIONcomment (about the spec title contradicting its own pseudocode) is removed alongside the upstream fix.4. Protocol variants — new
protocol_variants.tshelperobjects_lifecycle_test.md,objects_sync_test.mdandobjects_gc_test.mdall require every test to run once per protocol variant. A smalldescribeWithProtocolshelper (mirroring the legacy suite'sSharedHelper.testOnJsonMsgpackpattern at describe level) now generates a(json)and a(msgpack)suite per file — no command-line flags, environment variables, or CI changes needed; the existing mocha glob picks both up. Coverage goes from 12 to 24 spec-mandated executions across the three suites.Validation
All suites run green against the nonprod sandbox through the standard harness:
objects_faults: 5/5, with the restructured RTO20e observing 92008 (cause 90000) end-to-end through uts-proxy.objects_lifecycle/objects_sync/objects_gc: 24/24 across both protocol variants.Companion spec changes
uts/objects/integration/objects_gc_test.mdand the RTO4 title fix inobjects_sync_test.mdare authored on the spec branch and intended to accompany that PR, making everySource:reference in these test files resolvable.🤖 Generated with Claude Code
Summary by CodeRabbit
undefinedvsnull).FAILEDwhilepublishAndApplyis waiting, with precise error-code checks.