[Test] Stabilize message-translator tests on Node.js 26.4#1555
Merged
minggangw merged 3 commits intoJun 30, 2026
Conversation
There was a problem hiding this comment.
Pull request overview
This PR stabilizes the message-translator integration tests under Node.js 26.4 by eliminating a pub/sub discovery timing race where a single publish could be missed (especially with volatile QoS), causing long “hang-like” delays.
Changes:
- Republish test messages on a 100ms interval until the subscription receives the expected sample.
- Clear the republish timer on successful receipt (and in most failure paths) to stop ongoing publishes once the assertion completes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/test-message-translator-primitive.js | Adds periodic republishing and interval cleanup across primitive/array/large TypedArray translation tests to avoid the discovery-window race. |
| test/test-message-translator-complex.js | Adds periodic republishing for complex message translation tests to avoid missing the first publish during discovery. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
218
to
222
| @@ -219,6 +221,10 @@ describe('Rclnodejs message translation: complex types', function () { | |||
| reject('case ' + i + '. Expected: ' + v + ', Got: ' + value); | |||
| } | |||
|
coverage: 91.103%. remained the same — minggangw:fix-test-hang-nodejs-26.4 into RobotWebTools:develop |
| ); | ||
| } | ||
| }); | ||
| timer = setInterval(() => publisher.publish(v), 100); |
| ); | ||
| } | ||
| }); | ||
| timer = setInterval(() => publisher.publish({ data: v }), 100); |
| data: testData.values, | ||
| }); | ||
| }; | ||
| timer = setInterval(() => publisher.publish(msg), 100); |
| data: testData.values, | ||
| }); | ||
| }; | ||
| timer = setInterval(() => publisher.publish(msg), 100); |
| // Keep republishing until the subscription is matched and the | ||
| // message is received; a single publish can be lost while pub/sub | ||
| // discovery is still in progress. | ||
| timer = setInterval(() => publisher.publish(v), 100); |
Comment on lines
+550
to
561
| const start = Date.now(); | ||
| timer = setInterval(() => { | ||
| if (Date.now() - start > 55 * 1000) { | ||
| clearInterval(timer); | ||
| node.destroy(); | ||
| reject('Timed out waiting for message'); | ||
| return; | ||
| } | ||
| publisher.publish(msg); | ||
| }, 100); | ||
| publisher.publish(msg); | ||
| rclnodejs.spin(node); |
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
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.
The complex/primitive message-translator tests created a publisher and subscriber on one topic, published a single message, then spun. With volatile QoS that lone sample is only delivered if pub/sub discovery completes within the first DDS announcement window; when that window is missed, matching only recovers on the next full announcement cycle (~30s per case), making the CI suite appear to hang.
Republish on a 100ms interval until the subscription receives the message, bounded to 55s so the timer self-cleans before the Mocha timeout, and clear the interval + destroy the node on both the success and failure paths.
Fix: #1556