Skip to content

Fix old test that was skipped#2440

Merged
spomichter merged 4 commits into
mainfrom
Dreamsorcerer-patch-4
Jun 10, 2026
Merged

Fix old test that was skipped#2440
spomichter merged 4 commits into
mainfrom
Dreamsorcerer-patch-4

Conversation

@Dreamsorcerer

Copy link
Copy Markdown
Collaborator

No description provided.

@greptile-apps

greptile-apps Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR revives a previously skipped integration test for LCM publishing of TFMessage transforms. The skip marker is removed and the test is given a proper structure: a try/finally fixture for LCM lifecycle management, real subscriptions via CallbackCollector, and round-trip assertions on decoded messages.

  • The lcm fixture wraps lcm.start() / lcm.stop() in a try/finally block, correctly addressing the thread-leak concern raised on the prior version.
  • The bare time.sleep smoke test is replaced by collector.wait() with a 5-second timeout and assertions on the received TFMessage contents, giving the test real pass/fail semantics.
  • The dimos_lcm import that was previously guarded behind the skip marker is now handled naturally through TFMessage.py's own module-level import, removing the need for an in-function import guard.

Confidence Score: 5/5

The change is self-contained to a single test file and does not touch production code; the reworked test correctly manages LCM lifecycle and asserts on decoded message contents.

The fixture properly stops the LCM thread in its finally block, eliminating the thread-leak the autouse monitor_threads fixture would have caught. Assertions are correct: both publish calls use the right Transform objects and the expected child_frame_id lists match the constructed messages. No production code is changed.

No files require special attention.

Important Files Changed

Filename Overview
dimos/msgs/tf2_msgs/test_TFMessage_lcmpub.py Skipped smoke test replaced with a proper round-trip integration test: adds an LCM fixture with cleanup, subscribes before publishing, and asserts on decoded TFMessage contents using CallbackCollector.

Sequence Diagram

sequenceDiagram
    participant F as lcm fixture
    participant T as test_publish_transforms
    participant L as LCM instance
    participant C as CallbackCollector
    participant LT as LCM handler thread

    F->>L: "LCM(url=lcm_url)"
    F->>L: lcm.start()
    F-->>T: yield lcm

    T->>C: CallbackCollector(2)
    T->>L: lcm.subscribe(topic, collector)
    T->>L: lcm.publish(TFMessage(world_to_base, base_to_arm))
    T->>L: lcm.publish(TFMessage(world_to_base, arm_to_gripper))
    LT-->>C: callback(TFMessage, topic) [msg 1]
    LT-->>C: callback(TFMessage, topic) [msg 2]
    T->>C: collector.wait()
    T->>T: "assert len(results)==2"
    T->>T: assert child_frame_ids msg1
    T->>T: assert child_frame_ids msg2

    F->>L: lcm.stop() [finally]
Loading

Reviews (2): Last reviewed commit: "[autofix.ci] apply automated fixes" | Re-trigger Greptile

Comment thread dimos/msgs/tf2_msgs/test_TFMessage_lcmpub.py Outdated
@codecov

codecov Bot commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@Dreamsorcerer Dreamsorcerer changed the title Test if old test passes now Fix old test that was skipped Jun 9, 2026
@github-actions github-actions Bot added the ready-to-merge Required CI checks have passed on this PR label Jun 9, 2026
@spomichter spomichter merged commit fc79281 into main Jun 10, 2026
25 checks passed
@spomichter spomichter deleted the Dreamsorcerer-patch-4 branch June 10, 2026 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge Required CI checks have passed on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants