Skip to content

test(node-js): add unit tests for controller command logic; fix DriveValue.read rounding - #537

Merged
thias15 merged 1 commit into
ob-f:masterfrom
Arvin7liu:fix/nodejs-controller-command-tests
Aug 22, 2026
Merged

thias15 merged 1 commit into
ob-f:masterfrom
Arvin7liu:fix/nodejs-controller-command-tests

Conversation

@Arvin7liu

@Arvin7liu Arvin7liu commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

The node-js web controller has no automated tests for its server-side command logic, even though that logic computes the exact left/right motor commands sent to the robot. This PR adds dependency-free unit tests and fixes a latent rounding bug found while writing them.

  • New tests (controller/node-js/test/commands.test.js, run via Node's built-in node:test):
    • CommandHandler drive mapping for goForward / goBackward / forwardLeft / forwardRight / rotateLeft / rotateRight / reset.
    • DriveCommandReducer de-duplication (identical consecutive drive commands are not re-sent).
    • CommandHandler.sendCommand raw-command forwarding.
    • DriveValue clamping to [-1, 1] and read() precision.
  • Bug fix: DriveValue.read() used Math.round(value, 3). Math.round only accepts one argument, so the 3 was ignored and the method returned an integer instead of a 3-decimal value. Changed to Math.round(value * 1000) / 1000. DriveValue is now exported so the tests can reach it. Note: read() isn't currently called anywhere in server/ or client/ (the drive-command path uses the raw .min()/.max()/.write() values directly), so this fix doesn't change any live behavior today, it's a correctness fix to a previously-unused method, now exercised by this PR's own tests.
  • Added a "test": "node --test" script to controller/node-js/package.json (zero new dependencies; runs on Node 18+).

Why this matters

The CommandHandler drive mapping tested here is the code that computes the actual left/right motor commands sent to the robot, so a regression there would silently send wrong motor commands to a physical robot. The new tests lock in that expected behavior and run in CI-friendly, dependency-free fashion.

Test plan

  • cd controller/node-js && npm test → 10/10 passing on Node 22 (also verified with node --check on all server/*.js and client/*.js).
  • Regression check: reverting the read() fix makes the DriveValue test fail (proving the test actually guards the fix).

Notes

This is unrelated to #447 / PR #514 (the TF/Keras policy-training issue), which is handled separately.

🤖 Generated with WorkBuddy

…Value.read rounding

- Add controller/node-js/test/commands.test.js covering CommandHandler drive
  mapping, DriveCommandReducer de-duplication, and DriveValue bounds.
- Fix DriveValue.read(): Math.round(value, 3) ignored the 2nd argument and
  returned an integer; use Math.round(value * 1000) / 1000 for 3-decimal
  precision. Export DriveValue so it is testable.
- Add "test": "node --test" script (zero new dependencies, runs on Node 18+).
@thias15

thias15 commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator

Nice, dependency-free test coverage; traced the assertions by hand against CommandHandler/DriveValue and they're all correct, including the negative-rounding case.

One correction: DriveValue.read() isn't actually called anywhere in server/ or client/ today. sendDriveCommand uses the raw .min()/.max()/.write() values directly, not .read(). So the rounding fix wasn't live-affecting motor commands, it's a fix to a previously-unused method now exercised by this PR's own tests. I'll update the description to reflect that.

@thias15 thias15 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified all test assertions by hand against the actual CommandHandler/DriveValue logic; they're correct. CI is green. Approving.

@thias15
thias15 merged commit 3646fc3 into ob-f:master Aug 22, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants