Add serial and buttplugio integration tests - #86
Closed
heavyrubberslave wants to merge 23 commits into
Closed
heavyrubberslave wants to merge 23 commits into
heavyrubberslave wants to merge 23 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR refactors the server startup architecture to use explicit container creation instead of a service provider, migrates all serial device providers to use ChangesServer Architecture and Integration Test Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tests/integration/buttplugIoDevice.spec.ts (2)
173-189: ⚡ Quick winTest name references implementation detail (100ms interval).
The test name mentions "100ms interval," but this value is not visible in the test configuration. If the polling interval changes in the implementation, the test name will be misleading. Consider either:
- Making the interval configurable in the test setup so it's explicit
- Removing the specific timing from the test name (e.g., "polls sensor values automatically")
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/buttplugIoDevice.spec.ts` around lines 173 - 189, The test title hardcodes an implementation detail ("100ms interval"); update the spec to avoid brittle naming by either (A) making the poll interval explicit in the test setup (e.g., pass a configurable interval into the system under test or simulator and reference that value in the test name) so the name reflects the configured value, or (B) rename the spec's it(...) description to remove the specific timing (for example "polls sensor values automatically" or "polls sensor values at the configured interval") and leave the rest of the test (the simulator.addDevice call, device.on(DeviceEvent.deviceRefreshed) wait, and expect on device.getAttribute) unchanged. Ensure you reference the test's it(...) block and/or the simulator configuration when applying the change so the name matches the actual configuration.
142-152: 💤 Low valueConsider replacing the hardcoded delay with a more deterministic approach.
The 200ms
setTimeoutdelay introduces potential flakiness if the test environment is under load. While this may be acceptable for a simulator-based integration test, consider whether the simulator could expose a method to trigger the device addition immediately, or use a retry/poll mechanism instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/buttplugIoDevice.spec.ts` around lines 142 - 152, The test currently uses a fragile 200ms setTimeout before adding a device; instead make the addition deterministic by removing the sleep and either (a) triggering the simulator push immediately (call the simulator method that emits the device-add event before awaiting waitForDeviceConnected, e.g., invoke simulator.addDevice prior to awaiting deviceConnected or use a provided simulator.triggerPush method), or (b) implement an explicit retry/poll loop around waitForDeviceConnected that repeatedly checks for the device for a bounded time window. Update the test to call simulator.addDevice and then await deviceConnected (or replace the sleep with a bounded poll using waitForDeviceConnected) so the test no longer relies on a hardcoded timeout; reference helpers: waitForDeviceConnected, simulator.addDevice, and the deviceConnected variable when making the change.src/app.ts (1)
36-39: ⚡ Quick win
AppOptions.dataPathis stale and currently unused.
createApponly consumesallowedOrigins(Line 169), while path ownership already lives increateContainer(dataPath). KeepingdataPathinAppOptionsmakes the API contract misleading.♻️ Proposed cleanup
--- a/src/app.ts +++ b/src/app.ts @@ export interface AppOptions { allowedOrigins: string[]; - dataPath: string; }--- a/src/index.ts +++ b/src/index.ts @@ -const appOptions = { allowedOrigins, dataPath: env.DATA_PATH }; +const appOptions = { allowedOrigins };Also applies to: 168-170
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app.ts` around lines 36 - 39, AppOptions currently exposes a stale dataPath field that isn't used by createApp—remove dataPath from the AppOptions interface and any related type references, and update createApp to only accept/consume allowedOrigins; rely on createContainer(dataPath) to own path management. Specifically, delete the dataPath property from the AppOptions interface declaration (AppOptions), update any call sites or type annotations that construct or expect AppOptions to stop providing dataPath, and ensure createApp (the function that reads allowedOrigins) and createContainer(dataPath) remain the single sources of truth for path ownership.tests/integration/helpers/mockSerialPortFactory.ts (1)
22-33: Mock serial ports are already registered in the integration test setup, somockSerialPortFactorydoesn’t needcreatePort(...)
tests/integration/slvCtrlSerialDevice.spec.tscallsSerialPortMock.binding.createPort(TEST_PORT_PATH, ...)inbeforeAllbeforeannounceDetectedDevice(...).src/device/provider/serialDeviceProvider.tsopens the port (port.open(...)) only afterserialPortFactory.create(...)runs, so the mock binding exists by the timeopenis invoked.Optional: centralize/document that required
createPort(...)setup if more integration specs start announcing additional mock port paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/helpers/mockSerialPortFactory.ts` around lines 22 - 33, The create method currently attempts to register a mock port via SerialPortMock.binding.createPort(...) even though integration tests already register mock ports; remove the createPort(...) call from the public override create(...) implementation in mockSerialPortFactory so the factory only constructs the SerialPortMock, keeps the open listener that attaches the simulator (this.simulator.attachToPort(...)), and relies on tests to call SerialPortMock.binding.createPort(...) in their beforeAll; optionally add a short comment in create(...) noting that port registration is performed by the test setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app.ts`:
- Around line 203-204: The code currently calls
websocketServer.attach(httpServer) and websocketServer.attach(httpsServer) which
re-attaches the same Socket.IO Server instance to multiple Node servers;
instead, create separate Socket.IO Server instances for each HTTP/HTTPS server
(e.g., websocketServerHttp and websocketServerHttps) and attach each to its
respective server, replicate any per-server configuration/handlers from the
current websocketServer setup to both instances, and update shutdown logic to
close both servers (or otherwise coordinate disconnects) rather than relying on
a single io.close(); ensure you update any references to websocketServer in
connection/namespace handlers to use the appropriate new instance names.
In `@tests/integration/helpers/buttplugIoServerSimulator.ts`:
- Around line 88-99: The stop() method can hang because it always awaits
Promises whose callbacks never run when this.wss or this.server are null; modify
stop() to only await the close Promises when the corresponding server exists
(i.e., if (this.wss) await new Promise(...), and if (this.server) await new
Promise(...)), or alternatively short-circuit to resolve immediately when they
are null; keep the existing connectedClients clearing and ws.close() loop as-is
and ensure you reference the stop() method, this.wss, and this.server when
making the conditional checks.
---
Nitpick comments:
In `@src/app.ts`:
- Around line 36-39: AppOptions currently exposes a stale dataPath field that
isn't used by createApp—remove dataPath from the AppOptions interface and any
related type references, and update createApp to only accept/consume
allowedOrigins; rely on createContainer(dataPath) to own path management.
Specifically, delete the dataPath property from the AppOptions interface
declaration (AppOptions), update any call sites or type annotations that
construct or expect AppOptions to stop providing dataPath, and ensure createApp
(the function that reads allowedOrigins) and createContainer(dataPath) remain
the single sources of truth for path ownership.
In `@tests/integration/buttplugIoDevice.spec.ts`:
- Around line 173-189: The test title hardcodes an implementation detail ("100ms
interval"); update the spec to avoid brittle naming by either (A) making the
poll interval explicit in the test setup (e.g., pass a configurable interval
into the system under test or simulator and reference that value in the test
name) so the name reflects the configured value, or (B) rename the spec's
it(...) description to remove the specific timing (for example "polls sensor
values automatically" or "polls sensor values at the configured interval") and
leave the rest of the test (the simulator.addDevice call,
device.on(DeviceEvent.deviceRefreshed) wait, and expect on device.getAttribute)
unchanged. Ensure you reference the test's it(...) block and/or the simulator
configuration when applying the change so the name matches the actual
configuration.
- Around line 142-152: The test currently uses a fragile 200ms setTimeout before
adding a device; instead make the addition deterministic by removing the sleep
and either (a) triggering the simulator push immediately (call the simulator
method that emits the device-add event before awaiting waitForDeviceConnected,
e.g., invoke simulator.addDevice prior to awaiting deviceConnected or use a
provided simulator.triggerPush method), or (b) implement an explicit retry/poll
loop around waitForDeviceConnected that repeatedly checks for the device for a
bounded time window. Update the test to call simulator.addDevice and then await
deviceConnected (or replace the sleep with a bounded poll using
waitForDeviceConnected) so the test no longer relies on a hardcoded timeout;
reference helpers: waitForDeviceConnected, simulator.addDevice, and the
deviceConnected variable when making the change.
In `@tests/integration/helpers/mockSerialPortFactory.ts`:
- Around line 22-33: The create method currently attempts to register a mock
port via SerialPortMock.binding.createPort(...) even though integration tests
already register mock ports; remove the createPort(...) call from the public
override create(...) implementation in mockSerialPortFactory so the factory only
constructs the SerialPortMock, keeps the open listener that attaches the
simulator (this.simulator.attachToPort(...)), and relies on tests to call
SerialPortMock.binding.createPort(...) in their beforeAll; optionally add a
short comment in create(...) noting that port registration is performed by the
test setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1836cead-3b6b-4cac-815f-3988e3a67bde
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
package.jsonsrc/app.tssrc/device/protocol/buttplugIo/buttplugIoDevice.tssrc/device/protocol/buttplugIo/buttplugIoDeviceFactory.tssrc/device/protocol/estim2b/estim2bSerialDeviceProvider.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusDevice.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.tssrc/device/protocol/zc95/zc95SerialDeviceProvider.tssrc/device/provider/serialDeviceProvider.tssrc/factory/serialPortFactory.tssrc/index.tssrc/serviceMap.tssrc/serviceProvider/serverServiceProvider.tssrc/util/expressUtils.tstests/integration/api.spec.tstests/integration/automationScripts.spec.tstests/integration/buttplugIoDevice.spec.tstests/integration/deviceEvents.spec.tstests/integration/helpers/appHelper.tstests/integration/helpers/buttplugIoServerSimulator.tstests/integration/helpers/mockSerialPortFactory.tstests/integration/helpers/slvCtrlPlusDeviceSimulator.tstests/integration/slvCtrlSerialDevice.spec.ts
💤 Files with no reviewable changes (2)
- src/serviceProvider/serverServiceProvider.ts
- src/serviceMap.ts
Comment on lines
+88
to
+99
| public async stop(): Promise<void> { | ||
| for (const ws of this.connectedClients) { | ||
| ws.close(); | ||
| } | ||
| this.connectedClients.clear(); | ||
|
|
||
| await new Promise<void>((resolve, reject) => { | ||
| this.wss?.close(err => (err ? reject(err) : resolve())); | ||
| }); | ||
| await new Promise<void>((resolve, reject) => { | ||
| this.server?.close(err => (err ? reject(err) : resolve())); | ||
| }); |
There was a problem hiding this comment.
Prevent stop() from hanging when simulator wasn’t started.
At Line 94 and Line 97, the Promise resolution depends on callbacks that won’t run when this.wss or this.server is null, causing teardown hangs.
Proposed fix
public async stop(): Promise<void> {
for (const ws of this.connectedClients) {
ws.close();
}
this.connectedClients.clear();
- await new Promise<void>((resolve, reject) => {
- this.wss?.close(err => (err ? reject(err) : resolve()));
- });
- await new Promise<void>((resolve, reject) => {
- this.server?.close(err => (err ? reject(err) : resolve()));
- });
+ const wss = this.wss;
+ this.wss = null;
+ if (wss) {
+ await new Promise<void>((resolve, reject) => {
+ wss.close(err => (err ? reject(err) : resolve()));
+ });
+ }
+
+ const server = this.server;
+ this.server = null;
+ if (server) {
+ await new Promise<void>((resolve, reject) => {
+ server.close(err => (err ? reject(err) : resolve()));
+ });
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public async stop(): Promise<void> { | |
| for (const ws of this.connectedClients) { | |
| ws.close(); | |
| } | |
| this.connectedClients.clear(); | |
| await new Promise<void>((resolve, reject) => { | |
| this.wss?.close(err => (err ? reject(err) : resolve())); | |
| }); | |
| await new Promise<void>((resolve, reject) => { | |
| this.server?.close(err => (err ? reject(err) : resolve())); | |
| }); | |
| public async stop(): Promise<void> { | |
| for (const ws of this.connectedClients) { | |
| ws.close(); | |
| } | |
| this.connectedClients.clear(); | |
| const wss = this.wss; | |
| this.wss = null; | |
| if (wss) { | |
| await new Promise<void>((resolve, reject) => { | |
| wss.close(err => (err ? reject(err) : resolve())); | |
| }); | |
| } | |
| const server = this.server; | |
| this.server = null; | |
| if (server) { | |
| await new Promise<void>((resolve, reject) => { | |
| server.close(err => (err ? reject(err) : resolve())); | |
| }); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/helpers/buttplugIoServerSimulator.ts` around lines 88 - 99,
The stop() method can hang because it always awaits Promises whose callbacks
never run when this.wss or this.server are null; modify stop() to only await the
close Promises when the corresponding server exists (i.e., if (this.wss) await
new Promise(...), and if (this.server) await new Promise(...)), or alternatively
short-circuit to resolve immediately when they are null; keep the existing
connectedClients clearing and ws.close() loop as-is and ensure you reference the
stop() method, this.wss, and this.server when making the conditional checks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Chores