Skip to content

Fix possible LCM URL collision#2077

Open
Dreamsorcerer wants to merge 1 commit into
mainfrom
sam/lcm-urls
Open

Fix possible LCM URL collision#2077
Dreamsorcerer wants to merge 1 commit into
mainfrom
sam/lcm-urls

Conversation

@Dreamsorcerer
Copy link
Copy Markdown
Collaborator

@Dreamsorcerer Dreamsorcerer commented May 13, 2026

Couple of missed spots from #1901.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 13, 2026

Greptile Summary

This PR removes hardcoded isolated LCM multicast URLs from two test files and replaces them with the shared lcm_url fixture from dimos/conftest.py. This completes the work started in #1901 by ensuring all LCM tests use the xdist-worker-scoped URL instead of static addresses that could collide when multiple xdist workers run in parallel.

  • test_lcmpubsub.py: Removes _ISOLATED_LCM_URL = "udpm://239.255.76.98:7698?ttl=0" and threads the lcm_url fixture through all three fixtures; updates return type annotations from Generator[T, None, None] to Iterator[T].
  • test_pattern_sub.py: Removes _ISOLATED_LCM_URL = "udpm://239.255.76.99:7699?ttl=0", adds url: str parameters to context managers, updates the Case.pubsub_context callable type to Callable[[str], ...], and passes lcm_url through to every parametrized test function.

Confidence Score: 5/5

Safe to merge — changes are limited to test infrastructure and correct the xdist worker isolation for LCM multicast URLs.

Both files make the same mechanical substitution: replace a static hardcoded multicast address with the worker-scoped lcm_url fixture already provided by conftest.py. Type annotations are updated consistently. No production code is touched.

No files require special attention.

Important Files Changed

Filename Overview
dimos/protocol/pubsub/impl/test_lcmpubsub.py Removes hardcoded isolated multicast URL; fixture signatures updated to accept lcm_url: str and return Iterator[T] — straightforward and correct.
dimos/protocol/pubsub/test_pattern_sub.py Removes hardcoded isolated multicast URL; context-manager helpers and all test functions now accept and forward lcm_url; Case.pubsub_context type updated to Callable[[str], ...] to match — all changes are consistent and correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[pytest-xdist worker starts] --> B["conftest.py: compute _BUCKET\nhashlib.blake2b(worker_id) % 5000"]
    B --> C["Set LCM_DEFAULT_URL env var\nudpm://239.255.76.67:{7700+_BUCKET}?ttl=0"]
    B --> D["lcm_url fixture (session-scoped)\nreturns same URL"]
    D --> E[test_lcmpubsub.py fixtures]
    D --> F[test_pattern_sub.py test functions]
    E --> G[LCM instances with worker-specific URL]
    F --> H["lcm_typed_context(url) / lcm_bytes_context(url)"]
    H --> I[LCM pair with worker-specific URL]
    G --> J[Tests isolated per xdist worker]
    I --> J
    style J fill:#bfb,stroke:#333
Loading

Reviews (1): Last reviewed commit: "Fix possible LCM URL collision" | Re-trigger Greptile

@codecov
Copy link
Copy Markdown

codecov Bot commented May 13, 2026

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!

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.

1 participant