Skip to content

Add tests for the new Pose class - #888

Merged
Micheal Parks (micheal-parks) merged 4 commits into
mainfrom
pose-tests
Jul 28, 2026
Merged

Add tests for the new Pose class#888
Micheal Parks (micheal-parks) merged 4 commits into
mainfrom
pose-tests

Conversation

@micheal-parks

Copy link
Copy Markdown
Member

Overview

Adds a comprehensive set of unit tests for the new Pose class, giving the visualizer's core math library a solid bump in coverage.

@changeset-bot

changeset-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 5e404fb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@viamrobotics/motion-tools Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@micheal-parks

Copy link
Copy Markdown
Member Author

Claude (@claude) review

@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Claude finished Micheal Parks (@micheal-parks)'s task in 2m 58s —— View job


Review

  • Read CLAUDE.md and repo conventions
  • Read the test file (pose.spec.ts)
  • Read the Pose class implementation
  • Read Frame type for context
  • Post inline comments for line-specific issues
  • Post final summary

This is a well-structured test suite. The top-of-file comment explaining why round-trips alone aren't sufficient, and the deliberate use of absolute assertions to pin unit constants, is exactly the right approach. The it.each usage, the fields() snapshot helper, and the oZ-defaulting case matrix are all solid.

Issues

Two coverage gaps worth fixing (see inline comments):

  1. setFromObject3D only asserts x, not y/z (line 194) — the test name says "reads position back as millimetres" but only one axis is checked. An axis-swap bug would pass.

  2. ov_radians and ov_degrees tests only check theta (lines 382, 390) — the ov.copy() and ov.set() calls also copy the x/y/z components of the orientation vector. A regression that zeroed those fields would go undetected.

Minor

  • The destination-parameter variants of toMatrix4, toVector3, and toObject3D aren't exercised in the "shared scratch objects" suite — only toQuaternion(new Quaternion()) gets that treatment (line 295). These are the same default-parameter pattern, so a test like pose.toMatrix4(new Matrix4()) returning a correctly-written matrix would round out that group.

const pose = new Pose().setFromObject3D(object3D)

expect(pose.x).toBeCloseTo(1000, 6)
expect(pose.theta).toBeCloseTo(90, 6)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The test name promises "reads position back as millimetres" but only asserts x. y and z go through the same setFromVector3 path (each independently multiplied by M_TO_MM), so a bug like an axis swap would pass undetected here.

Suggested change
expect(pose.theta).toBeCloseTo(90, 6)
expect(pose.x).toBeCloseTo(1000, 6)
expect(pose.y).toBeCloseTo(2000, 6)
expect(pose.z).toBeCloseTo(3000, 6)
expect(pose.theta).toBeCloseTo(90, 6)

it('converts ov_radians into stored degrees', () => {
const pose = new Pose().setFromFrame({
orientation: { type: 'ov_radians', value: { x: 0, y: 0, z: 1, th: Math.PI / 2 } },
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Only theta is checked. The ov_radians path calls ov.copy(frame.orientation.value), which copies x, y, and z too — but a regression that dropped those fields would sail through here. Worth adding the full orientation assertion:

Suggested change
})
const pose = new Pose().setFromFrame({
orientation: { type: 'ov_radians', value: { x: 0, y: 0, z: 1, th: Math.PI / 2 } },
})
expect(pose.oX).toBeCloseTo(0, 6)
expect(pose.oY).toBeCloseTo(0, 6)
expect(pose.oZ).toBeCloseTo(1, 6)
expect(pose.theta).toBeCloseTo(90, 6)

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-28 20:26 UTC

@micheal-parks
Micheal Parks (micheal-parks) merged commit 1c61c5c into main Jul 28, 2026
12 checks passed
@micheal-parks
Micheal Parks (micheal-parks) deleted the pose-tests branch July 28, 2026 20:25
@claude claude Bot mentioned this pull request Jul 28, 2026
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