Skip to content

fix(pytest): autoconf#1325

Merged
leshy merged 1 commit intodevfrom
paul/fix/pytest-autoconf
Feb 21, 2026
Merged

fix(pytest): autoconf#1325
leshy merged 1 commit intodevfrom
paul/fix/pytest-autoconf

Conversation

@paul-nechifor
Copy link
Contributor

  • Make pytest dimos run sudo commands without -s by just temporarily disabling capture when executing sudo.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 20, 2026

Greptile Summary

Centralizes autoconf() execution into a session-scoped pytest fixture with suspended output capture to allow users to see and respond to sudo prompts. Removes redundant pubsub.lcm.autoconf() calls from individual test files.

  • Adds _autoconf fixture in conftest.py that runs once per test session with capture temporarily disabled
  • Removes duplicate autoconf() calls from two test modules along with unused pubsub imports
  • Missing exception handling: autoconf() can raise SystemExit(1) if critical checks fail, which should be wrapped in try/finally to ensure capture is always resumed

Confidence Score: 4/5

  • Safe to merge with one logical issue that should be addressed
  • The PR successfully centralizes autoconf() and removes code duplication, but the fixture lacks proper exception handling for SystemExit which could leave pytest's capture manager in an inconsistent state
  • Pay attention to dimos/conftest.py - add try/finally for exception safety

Important Files Changed

Filename Overview
dimos/conftest.py Adds session-scoped fixture to run autoconf() with suspended capture for sudo visibility; potential exception handling issue
dimos/perception/experimental/temporal_memory/test_temporal_memory_module.py Removed redundant pubsub.lcm.autoconf() call and unused import - now handled by conftest fixture
dimos/perception/test_spatial_memory_module.py Removed redundant pubsub.lcm.autoconf() call and unused import - now handled by conftest fixture

Sequence Diagram

sequenceDiagram
    participant Pytest
    participant conftest
    participant CaptureManager
    participant autoconf
    participant SystemConfigurator
    participant User

    Pytest->>conftest: session start
    conftest->>conftest: _autoconf fixture (autouse=True)
    conftest->>CaptureManager: suspend_global_capture(in_=True)
    Note over CaptureManager: Output capture disabled
    conftest->>autoconf: autoconf()
    autoconf->>SystemConfigurator: check system config
    alt Config needed
        SystemConfigurator->>User: print sudo commands
        SystemConfigurator->>User: input("Apply changes? [y/N]")
        User-->>SystemConfigurator: response
        alt User accepts
            SystemConfigurator->>SystemConfigurator: sudo run commands
        else User rejects critical fix
            SystemConfigurator-->>autoconf: raise SystemExit(1)
            autoconf-->>conftest: exception propagates
            Note over conftest: ⚠️ resume_global_capture() skipped
        end
    end
    autoconf-->>conftest: return
    conftest->>CaptureManager: resume_global_capture()
    Note over CaptureManager: Output capture re-enabled
    conftest-->>Pytest: fixture ready
Loading

Last reviewed commit: d1c8aef

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.

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@paul-nechifor paul-nechifor force-pushed the paul/fix/pytest-autoconf branch from d1c8aef to b40b021 Compare February 20, 2026 22:55
Copy link
Contributor

@leshy leshy left a comment

Choose a reason for hiding this comment

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

tested, awesome thanks

@leshy leshy merged commit 2706124 into dev Feb 21, 2026
16 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