Test that the device factory carries over the plugins - #2291
Conversation
PubSubDevice.createClient copies the caller's options, and nothing checked that the plugin hook survives that copy. Losing it is invisible at construction and fatal later: a client built without it traps on first access to channel.object, so every LiveObjects application reaching Ably through the factory would break. The test reuses PluginAPITests' mock plugin, so the AblyTests target needs no dependency on AblyLiveObjects.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe pull request adds a test for ChangesPlugin option propagation
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change adds focused test coverage for plugin option propagation without introducing production behavior changes. 🚥 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. A rabbit checks the plugin trail Comment |
There was a problem hiding this comment.
🟢 Approval recommended
The change adds focused regression coverage with no unresolved issues.
Pull request overview
Adds regression coverage verifying PubSubDevice.createClient(options:) preserves configured plugins.
Changes:
- Reuses the existing mock plugin.
- Confirms the copied client retains the plugin entry.
File summaries
| File | Description |
|---|---|
Test/AblyTests/Tests/PubSubDeviceTests.swift |
Adds plugin-copy regression coverage. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Stacked on #2265, and addresses the MAJOR finding on it (#2265 (comment)).
PubSubDevice.createClient(options:)works by copying the caller's options, and nothing checked thatARTClientOptions.pluginssurvives that copy. Losing it is invisible at construction and fatal later: a client built without the plugin traps on first access tochannel.object, so every LiveObjects application reaching Ably through the factory would break.test__011__createClient__carries_over_the_pluginsputs a plugin in the options, builds through the factory, and reads the plugins back off the client. It sits with the five other contract tests for the same call and uses their idiom,client.internal.options.No manifest change was needed. The finding expected
AblyTeststo gain a dependency onAblyLiveObjects; instead the test reusesPluginAPITests.MockLiveObjectsPlugin, which already exists in this target and already conforms to the realLiveObjectsPluginProtocol. So the core SDK's test target stays clear of the plugin, and the test needs no network — it runs in every existing lane on all three platforms.Checked that it can fail: with
options.plugins = self.plugins;removed fromARTClientOptions.m'scopyWithZone:, it fails withXCTUnwrap failed: expected non-nil value of type "Dictionary<ARTPluginName, Any>".The behavioural half of the finding — that
channel.objectworks on a factory-built client — is #2292, at the top of the stack where the LiveObjects test target already depends onAblyPubSubDevice.Summary by CodeRabbit