Fix: correct bugs in test files#1418
Conversation
There was a problem hiding this comment.
Pull request overview
This PR focuses on correcting issues in the repository’s test suite, primarily by removing/repairing incorrect assertions and fixing a couple of test logic mistakes.
Changes:
- Fix incorrect/ineffective assertions in multiple tests (e.g.,
assert.ok(a, b)→assert.strictEqual(a, b)). - Correct test logic/data errors (parameter name mismatch; lifecycle transition index).
- Add an average sleep-time bound to the rate accuracy test; remove a stray/invalid line in type tests and an empty test case.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/types/index.test-d.ts | Removes an incorrect type assertion line. |
| test/test-utils.js | Removes an empty/no-op test. |
| test/test-rate.js | Adds an assertion bounding average rate sleep time. |
| test/test-parameter-service.js | Fixes incorrect parameter name used when decoding the response. |
| test/test-lifecycle.js | Fixes incorrect transition index being asserted. |
| test/test-action-server.js | Fixes multiple incorrect assertions; marks goal-expiration tests as skipped with TODO notes. |
| test/client_setup.js | Removes a call to an undefined timer.cancel() in SIGINT handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // avg sleep time should be within a reasonable range of the expected period | ||
| const expectedPeriod = 1000 / hz; // 1ms | ||
| assert.ok( | ||
| avg < expectedPeriod * 5, | ||
| `Average sleep time ${avg}ms exceeded 5x the expected period ${expectedPeriod}ms` | ||
| ); |
There was a problem hiding this comment.
avg is measured from before rate.sleep() until after rclnodejs.spinOnce(node, 0), so it includes both the sleep duration and the spin time. Comparing that combined value against the expected sleep period can make this test inaccurate/flaky; consider measuring end immediately after await rate.sleep() (or otherwise accounting for spinOnce overhead) so the assertion reflects what it intends to validate.
test/test-action-server.js
Outdated
| // TODO: This test was always passing due to tautological assert.ok(size, 0). | ||
| // With proper assert.strictEqual, it reveals goal expiration doesn't remove | ||
| // handles from _goalHandles. Needs investigation of the native expire mechanism. | ||
| it.skip('Test expire goals single', async function () { |
There was a problem hiding this comment.
Using it.skip(...) here disables coverage for goal-expiration behavior and can mask regressions. Prefer keeping the test enabled by either (a) updating the expected behavior if _goalHandles is not supposed to shrink on expiration, or (b) fixing the underlying expiration/cleanup so the assertion holds. If a skip is unavoidable, please gate it with a concrete condition and link to a tracking issue so it gets re-enabled.
test/test-action-server.js
Outdated
| // TODO: Same as above - tautological assert.ok was hiding that goal | ||
| // expiration doesn't remove handles from _goalHandles. | ||
| it.skip('Test expire goals multi', async function () { |
There was a problem hiding this comment.
Same concern as the skipped single-expiration test: it.skip(...) removes coverage for multi-goal expiration. Please either align the assertion with the intended behavior or fix expiration cleanup so this can run reliably; otherwise add a conditional skip with a linked issue for follow-up.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
### Summary
Fixed 6 bugs across 6 test files discovered during code review. All issues were in test code only — no library changes.
### Changes
#### 1. `test/client_setup.js` — Remove reference to undefined `timer`
The `SIGINT` handler called `timer.cancel()`, but `timer` is not defined in this scope. Removed the call to prevent a `ReferenceError` if the signal is received.
#### 2. `test/test-lifecycle.js` — Fix duplicate transition index
Two consecutive assertions both checked `transitions[1]`, meaning `transitions[2]` was never validated. Changed the second check from `transitions[1]` to `transitions[2]`.
#### 3. `test/test-parameter-service.js` — Fix wrong parameter name
`Parameter.fromParameterMessage` was called with `name: 'p1'` but the test was validating parameter `A.p3`. Changed to `name: 'A.p3'`.
#### 4. `test/test-rate.js` — Add missing assertion
The test computed an average sleep time but never asserted on it. Added an assertion that the average is within 5x the expected period.
#### 5. `test/test-utils.js` — Remove empty duplicate test
An empty test body `it('should valid ensureDirSync works correctly', function () {})` duplicated the immediately following test `it('should verify ensureDirSync works correctly', ...)`. Removed the empty duplicate.
#### 6. `test/types/index.test-d.ts` — Remove misplaced type assertion
`expectType<boolean>(client.isServiceServerAvailable())` was in the **ActionClient** section but tested `client` (the service client variable). The same assertion already exists in the correct Client section. Removed the misplaced copy.
### Files changed
| File | Change |
|------|--------|
| `test/client_setup.js` | Remove undefined `timer.cancel()` |
| `test/test-lifecycle.js` | `transitions[1]` → `transitions[2]` in second assertion |
| `test/test-parameter-service.js` | `'p1'` → `'A.p3'` |
| `test/test-rate.js` | Add avg sleep time assertion |
| `test/test-utils.js` | Remove empty duplicate test |
| `test/types/index.test-d.ts` | Remove misplaced `client.isServiceServerAvailable()` |
Fix: #1417
Summary
Fixed 6 bugs across 6 test files discovered during code review. All issues were in test code only — no library changes.
Changes
1.
test/client_setup.js— Remove reference to undefinedtimerThe
SIGINThandler calledtimer.cancel(), buttimeris not defined in this scope. Removed the call to prevent aReferenceErrorif the signal is received.2.
test/test-lifecycle.js— Fix duplicate transition indexTwo consecutive assertions both checked
transitions[1], meaningtransitions[2]was never validated. Changed the second check fromtransitions[1]totransitions[2].3.
test/test-parameter-service.js— Fix wrong parameter nameParameter.fromParameterMessagewas called withname: 'p1'but the test was validating parameterA.p3. Changed toname: 'A.p3'.4.
test/test-rate.js— Add missing assertionThe test computed an average sleep time but never asserted on it. Added an assertion that the average is within 5x the expected period.
5.
test/test-utils.js— Remove empty duplicate testAn empty test body
it('should valid ensureDirSync works correctly', function () {})duplicated the immediately following testit('should verify ensureDirSync works correctly', ...). Removed the empty duplicate.6.
test/types/index.test-d.ts— Remove misplaced type assertionexpectType<boolean>(client.isServiceServerAvailable())was in the ActionClient section but testedclient(the service client variable). The same assertion already exists in the correct Client section. Removed the misplaced copy.Files changed
test/client_setup.jstimer.cancel()test/test-lifecycle.jstransitions[1]→transitions[2]in second assertiontest/test-parameter-service.js'p1'→'A.p3'test/test-rate.jstest/test-utils.jstest/types/index.test-d.tsclient.isServiceServerAvailable()Fix: #1417