[ECO-5553] Fixed omitted tests responsible for handling vcdiff encoding #1161
Conversation
WalkthroughRe-enabled and modernized unit tests across realtime and rest suites, moved delta codec configuration from channel-name encoding to ChannelOptions.params with runtime verification, tightened generics, and updated assertions (including assertThrows and order-insensitive checks). No public API signatures changed except Channels#get overload usage in tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test
participant Ably as Ably Client
participant Channels as Channels
participant Channel as Channel
Note over Test,Ably: Setup
Test->>Ably: create client
Note over Test,Channels: Acquire channel using options (new)
Test->>Channels: get(name, ChannelOptions{ params: { delta: "vcdiff" } })
Channels->>Channel: return Channel instance
Test->>Channel: attach()
Channel-->>Test: attached
Test->>Channel: getParams()
Channel-->>Test: { delta: "vcdiff", ... }
Note right of Test: Assert params include delta=vcdiff
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks (2 passed, 2 warnings, 1 inconclusive)❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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 |
github-actions
bot
temporarily deployed
to
staging/pull/1161/features
github-actions
bot
temporarily deployed
to
staging/pull/1161/javadoc
sacOO7
force-pushed
the
fix/vcdiff-encoding
branch
from
8d48be7 to
a12d9cb
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1161/features
github-actions
bot
temporarily deployed
to
staging/pull/1161/javadoc
sacOO7
force-pushed
the
fix/vcdiff-encoding
branch
from
a12d9cb to
f22a0f8
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1161/features
github-actions
bot
temporarily deployed
to
staging/pull/1161/javadoc
sacOO7
force-pushed
the
fix/vcdiff-encoding
branch
from
f22a0f8 to
8c85761
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1161/features
github-actions
bot
temporarily deployed
to
staging/pull/1161/javadoc
github-actions
bot
temporarily deployed
to
staging/pull/1161/features
github-actions
bot
temporarily deployed
to
staging/pull/1161/javadoc
for modes and params
sacOO7
force-pushed
the
fix/vcdiff-encoding
branch
from
2442315 to
754a064
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1161/features
github-actions
bot
temporarily deployed
to
staging/pull/1161/javadoc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
lib/src/test/java/io/ably/lib/test/rest/RestPresenceTest.java (6)
90-106: Re-enabled test looks good; swap expected/actual in size assert for clearer failuresUse expected first to improve assertion diffs.
- assertEquals("Expected 4 messages", members.items().length, 4); + assertEquals("Expected 4 messages", 4, members.items().length);
117-133: Same here: reverse the order in the size assertKeeps assertion messages consistent project-wide.
- assertEquals("Expected 4 messages", members.items().length, 4); + assertEquals("Expected 4 messages", 4, members.items().length);
144-160: Forward/limit: fix assert orderMinor readability tweak only.
- assertEquals("Expected 2 messages", members.items().length, 2); + assertEquals("Expected 2 messages", 2, members.items().length);
171-187: Backward/limit: fix assert orderSame reasoning as above.
- assertEquals("Expected 2 messages", members.items().length, 2); + assertEquals("Expected 2 messages", 2, members.items().length);
198-260: Paginate forwards: normalize assertion ordering across pagesAlso fix the terminal page check.
- assertEquals("Expected 1 message", members.items().length, 1); + assertEquals("Expected 1 message", 1, members.items().length); - assertEquals("Expected 1 message", members.items().length, 1); + assertEquals("Expected 1 message", 1, members.items().length); - assertEquals("Expected 1 message", members.items().length, 1); + assertEquals("Expected 1 message", 1, members.items().length); - assertEquals("Expected 1 message", members.items().length, 1); + assertEquals("Expected 1 message", 1, members.items().length); - assertEquals("Expected no further members", members.items().length, 0); + assertEquals("Expected no further members", 0, members.items().length);
266-328: Paginate backwards: assertion order + tiny grammar and fail-name fixes
- Put expected first.
- “1 messages” → “1 message”.
- Fail message suffix should match the test name (“…_text_b”).
- assertEquals("Expected 1 message", members.items().length, 1); + assertEquals("Expected 1 message", 1, members.items().length); - assertEquals("Expected 1 messages", members.items().length, 1); + assertEquals("Expected 1 message", 1, members.items().length); - assertEquals("Expected 1 message", members.items().length, 1); + assertEquals("Expected 1 message", 1, members.items().length); - assertEquals("Expected 1 message", members.items().length, 1); + assertEquals("Expected 1 message", 1, members.items().length); - fail("rest_presencehistory_paginate_text_f: Unexpected exception"); + fail("rest_presencehistory_paginate_text_b: Unexpected exception");lib/src/test/java/io/ably/lib/test/realtime/RealtimeDeltaDecoderTest.java (1)
57-58: Prefer ChannelOptions over bracketed channel name for delta=vcdiffAlign this test with others and avoid the legacy “[?param=…]” syntax.
- Channel channel = ably.channels.get("[?delta=vcdiff]" + testName); + ChannelOptions options = new ChannelOptions(); + options.params = Map.of("delta", "vcdiff"); + Channel channel = ably.channels.get(testName, options);lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java (1)
287-291: Swap expected/actual in params assert for consistencyEverywhere else uses expected first.
- assertEquals("Verify channel params", channel.getParams(), options.params); + assertEquals("Verify channel params", options.params, channel.getParams());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java(17 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeDeltaDecoderTest.java(4 hunks)lib/src/test/java/io/ably/lib/test/rest/RestPresenceTest.java(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeDeltaDecoderTest.java (1)
lib/src/main/java/io/ably/lib/types/ChannelOptions.java (1)
ChannelOptions(13-131)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-realtime
🔇 Additional comments (8)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeDeltaDecoderTest.java (1)
97-108: Good move to ChannelOptions and param verificationThis matches TB2c intent and keeps tests consistent. LGTM.
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java (7)
35-35: Import Arrays: OKNeeded for containsInAnyOrder assertions on modes.
50-50: Static assertThrows import: OKModernizes exception assertions cleanly.
167-178: Attach with params via channels.get: OKParam and modes assertions look correct.
200-212: setOptions path: OKValidates setOptions flow; assertions are sound.
234-252: Re-get should throw when it would reattach: verify error codes are stableThe message check is good; codes 400/40000 may vary by transport/runtime. Please confirm across CI targets.
324-329: Modes order-insensitive check: OKLength + containsInAnyOrder is robust.
351-358: Delta param + modes together: OKCovers params+modes interplay well.
Also applies to: 365-368
lib/src/test/java/io/ably/lib/test/realtime/RealtimeDeltaDecoderTest.java
Outdated
Show resolved
Hide resolved
…tions.singletonMap
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeDeltaDecoderTest.java (1)
49-58: Prefer configuring delta via ChannelOptions over bracketed channel names.To keep tests consistent with the rest of the suite and future-proof against deprecated “[?...]” name-params, set params on ChannelOptions here too.
- Channel channel = ably.channels.get("[?delta=vcdiff]" + testName); + ChannelOptions chOpts = new ChannelOptions(); + chOpts.params = Collections.singletonMap("delta", "vcdiff"); + Channel channel = ably.channels.get(testName, chOpts);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java(18 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeDeltaDecoderTest.java(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeDeltaDecoderTest.java (1)
lib/src/main/java/io/ably/lib/types/ChannelOptions.java (1)
ChannelOptions(13-131)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check
- GitHub Check: check-liveobjects
- GitHub Check: check-rest
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest-okhttp
- GitHub Check: check (29)
- GitHub Check: check-realtime
- GitHub Check: check (19)
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: build
- GitHub Check: check (21)
🔇 Additional comments (11)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeDeltaDecoderTest.java (3)
14-24: Imports look good and Java 8–compatible.Switch to ChannelOptions and Collections avoids Map.of and keeps tests Java 8–safe.
97-101: LGTM: ChannelOptions-based delta configuration.Clear, minimal, and aligns with the direction taken elsewhere in the PR.
107-107: LGTM: Param assertion.Asserting getParams() against a singletonMap is correct and order-agnostic.
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java (8)
34-34: Import Arrays is appropriate.Used for containsInAnyOrder assertions and list comparisons below.
167-178: LGTM: Channels#get with ChannelOptions + params.Good use of params and modes; assertion order is expected vs actual.
200-213: LGTM: setOptions path mirrors Channels#get path.Covers both ways of applying params; assertions are consistent.
289-291: LGTM: Modes precedence covered.Good assertion that params.modes overrides options.modes; order-insensitive check is correct.
324-328: LGTM: Exact modes set validated.Length + containsInAnyOrder makes the assertion robust.
351-368: LGTM: Combining delta param with modes.Param equality and mode checks look correct.
2564-2573: LGTM: Explicit state transition sequences.The list-based assertions document expected sequences crisply.
50-50: No action needed: The version catalog (gradle/libs.versions.toml) defines JUnit as 4.13.2, so assertThrows is supported.
Summary by CodeRabbit