Skip to content

Comments

fix(tests): replace sleep() with threading.Event in pubsub tests#1350

Merged
paul-nechifor merged 1 commit intodevfrom
ivan/fix/pubsub-test-timeouts
Feb 23, 2026
Merged

fix(tests): replace sleep() with threading.Event in pubsub tests#1350
paul-nechifor merged 1 commit intodevfrom
ivan/fix/pubsub-test-timeouts

Conversation

@leshy
Copy link
Contributor

@leshy leshy commented Feb 23, 2026

Problem

Pubsub test_spec.py was flakey (https://github.com/dimensionalOS/dimos/actions/runs/22268096362/job/64417900362)

No matching Linear issue

Solution

Replaced time.sleep() waits with threading.Event in three tests:

  • test_store: single event, set on callback
  • test_multiple_subscribers: one event per subscriber
  • test_multiple_messages: event set when expected count reached

Each uses event.wait(timeout=1.0) so tests fail clearly on timeout instead of hanging

Breaking Changes

None

How to Test

pytest dimos/protocol/pubsub/test_spec.py -v

Contributor License Agreement

  • I have read and approved the CLA.

Busy-wait loops with no timeout could hang forever. Use threading.Event
for efficient blocking with 1s timeouts that fail clearly on expiry.
@leshy leshy changed the title fix(tests): replace busy-wait loops with threading.Event in pubsub tests fix(tests): replace sleep() with threading.Event in pubsub tests Feb 23, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 23, 2026

Greptile Summary

Replaces inefficient busy-wait loops with threading.Event for message synchronization in three pubsub tests (test_store, test_multiple_subscribers, test_multiple_messages), adding proper 1-second timeouts to prevent indefinite hangs.

  • Removed time.sleep() polling in favor of event.wait(timeout=1.0)
  • Each test now has clear timeout behavior with descriptive error messages
  • Removed debug print statement from test_store

Confidence Score: 4/5

  • Safe to merge with minimal risk - improves test reliability
  • Clean refactoring of test synchronization primitives with proper timeout handling, though one edge case around concurrent callback execution could theoretically occur
  • No files require special attention

Important Files Changed

Filename Overview
dimos/protocol/pubsub/test_spec.py Replaced busy-wait loops with threading.Event for cleaner timeout handling in pubsub tests

Last reviewed commit: 522b50d

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@paul-nechifor paul-nechifor merged commit 914a090 into dev Feb 23, 2026
42 of 46 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