Add tests to improve coverage #1363
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive test coverage across multiple areas of the rclnodejs library. The changes introduce 15 new or modified test files that significantly expand validation, error handling, and edge case testing.
Key Changes:
- Added extensive validation error tests for messages and parameters
- Introduced comprehensive utility function tests
- Added error path coverage for node operations, logging, and client-service interactions
- Extended existing test files with additional test cases for time sources, guard conditions, and async clients
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test-validation-errors.js | New comprehensive tests for message validation errors including type mismatches, range validation, and array validation |
| test/test-utils.js | New tests for utility functions including file operations, version comparison, and node name normalization |
| test/test-time-source.js | Extended existing tests with new cases for ROS time activation and node attachment/detachment |
| test/test-rmw.js | New tests for RMW (ROS Middleware) utility functions and environment variable handling |
| test/test-parameters.js | Extended parameter tests with comprehensive descriptor tests, ranges, and special value handling |
| test/test-parameter-validation-errors.js | New extensive tests for parameter validation including type checking, range validation, and read-only enforcement |
| test/test-node-error-paths.js | New comprehensive error path tests for node creation, publishers, subscriptions, services, and lifecycle management |
| test/test-message-validation.js | Extended message validation tests with additional message type handling |
| test/test-message-serialization-coverage.js | New comprehensive tests for message serialization covering TypedArrays, JSON conversion, and edge cases |
| test/test-logging-coverage.js | New extensive logging tests covering severity levels, conditional logging, and logger hierarchy |
| test/test-guard-condition.js | Extended tests with context handling and callback getter tests |
| test/test-clock-event.js | Modified copyright header and updated comments |
| test/test-client-service-coverage.js | New comprehensive client-service tests covering creation, communication, error handling, and QoS variations |
| test/test-async-client.js | Extended with AbortSignal polyfill tests and validation method tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/test-client-service-coverage.js
Outdated
| it('should send request and receive response', async function () { | ||
| const serviceName = `add_service_${Date.now()}`; | ||
|
|
||
| const service = serviceNode.createService( |
There was a problem hiding this comment.
Unused variable service.
| const service = serviceNode.createService( | |
| serviceNode.createService( |
test/test-client-service-coverage.js
Outdated
| it('should handle multiple requests', async function () { | ||
| const serviceName = `multi_service_${Date.now()}`; | ||
|
|
||
| const service = serviceNode.createService( |
There was a problem hiding this comment.
Unused variable service.
test/test-client-service-coverage.js
Outdated
| it('should handle zero values', async function () { | ||
| const serviceName = `zero_service_${Date.now()}`; | ||
|
|
||
| const service = serviceNode.createService( |
There was a problem hiding this comment.
Unused variable service.
test/test-client-service-coverage.js
Outdated
| it('should handle negative values', async function () { | ||
| const serviceName = `negative_service_${Date.now()}`; | ||
|
|
||
| const service = serviceNode.createService( |
There was a problem hiding this comment.
Unused variable service.
test/test-client-service-coverage.js
Outdated
| it('should handle large values', async function () { | ||
| const serviceName = `large_service_${Date.now()}`; | ||
|
|
||
| const service = serviceNode.createService( |
There was a problem hiding this comment.
Unused variable service.
test/test-node-error-paths.js
Outdated
|
|
||
| it('should clean up publishers on node destroy', function () { | ||
| const node = rclnodejs.createNode('cleanup_pub_test'); | ||
| const pub = node.createPublisher('std_msgs/msg/String', 'test_topic'); |
There was a problem hiding this comment.
Unused variable pub.
test/test-node-error-paths.js
Outdated
|
|
||
| it('should clean up subscriptions on node destroy', function () { | ||
| const node = rclnodejs.createNode('cleanup_sub_test'); | ||
| const sub = node.createSubscription( |
There was a problem hiding this comment.
Unused variable sub.
test/test-node-error-paths.js
Outdated
|
|
||
| it('should clean up timers on node destroy', function () { | ||
| const node = rclnodejs.createNode('cleanup_timer_test'); | ||
| const timer = node.createTimer(100, () => {}); |
There was a problem hiding this comment.
Unused variable timer.
| const timer = node.createTimer(100, () => {}); | |
| node.createTimer(100, () => {}); |
test/test-node-error-paths.js
Outdated
|
|
||
| it('should clean up clients on node destroy', function () { | ||
| const node = rclnodejs.createNode('cleanup_client_test'); | ||
| const client = node.createClient( |
There was a problem hiding this comment.
Unused variable client.
| const client = node.createClient( | |
| node.createClient( |
test/test-node-error-paths.js
Outdated
|
|
||
| it('should clean up services on node destroy', function () { | ||
| const node = rclnodejs.createNode('cleanup_service_test'); | ||
| const service = node.createService( |
There was a problem hiding this comment.
Unused variable service.
| const service = node.createService( | |
| node.createService( |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/test-utils-lib.js
Outdated
| await utils.ensureDir(dir); | ||
| }); | ||
|
|
||
| it('should valid ensureDirSync works correctly', function () { |
There was a problem hiding this comment.
The test description has grammatically incorrect wording. It should be "should verify that ensureDirSync works correctly" or "should validate that ensureDirSync works correctly" instead of "should valid ensureDirSync works correctly".
| utils.ensureDirSync(dir); | ||
| }); | ||
|
|
||
| it('should valid emptyDir works correctly', async function () { |
There was a problem hiding this comment.
The test description has grammatically incorrect wording. It should be "should verify that emptyDir works correctly" or "should validate that emptyDir works correctly" instead of "should valid emptyDir works correctly".
2 participants
Summary
Enhanced unit test coverage for core library modules and simplified CI configuration.
Details
Test Coverage Improvements:
utils.js,time_source.js,native_loader.js,timer.js,message_validation.js,message_serialization.js,client.js,event_handler.js, andlogging.js.stubOptionalfor newer API features likesetOnResetCallback).CI Configuration:
.github/workflows/linux-x64-build-and-test.ymlto remove the redundantfinishjob andparallelCoveralls upload, streamlining the process for single-job coverage reporting.Fix: #1362