Skip to content

Split Direct MARL space and lifecycle tests#6574

Open
nvsekkin wants to merge 9 commits into
isaac-sim:developfrom
nvsekkin:esekkin/direct-marl-test-split
Open

Split Direct MARL space and lifecycle tests#6574
nvsekkin wants to merge 9 commits into
isaac-sim:developfrom
nvsekkin:esekkin/direct-marl-test-split

Conversation

@nvsekkin

@nvsekkin nvsekkin commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Reuse the config-only make_empty_direct_marl_env_cfg fixture in the simulator-backed Direct MARL lifecycle test.
  • Add a kitless CPU unit test for agent counts and Gymnasium space construction.
  • Retain CPU/CUDA initialization, constructor-to-space wiring, and SimulationContext cleanup coverage.

This is PR 4 of the focused replacement series for draft #6526.

Type of change

  • Test refactor (no production behavior change)

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

Reuse the shared config-only factory and separate kitless space checks
from simulator lifecycle coverage.
@github-actions github-actions Bot added the isaac-lab Related to Isaac Lab team label Jul 16, 2026
@greptile-apps

greptile-apps Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR splits the single test_direct_marl_env.py integration test into two: a simulator-backed lifecycle test (refactored) and a new kitless unit test (test_direct_marl_env_spaces.py) that exercises agent-count and Gymnasium-space construction without launching Isaac Sim.

  • test_direct_marl_env.py: Replaces the inline helper with the shared make_empty_direct_marl_env_cfg fixture and adds post-close assertions, but moves the DirectMARLEnv constructor call outside the try/finally block.
  • test_direct_marl_env_spaces.py: Creates a DirectMARLEnv via object.__new__, stubs scene and sim with SimpleNamespace, and calls _configure_env_spaces() directly to validate space shapes without starting the simulator.

Confidence Score: 3/5

The refactored lifecycle test has a gap: if DirectMARLEnv construction throws, close() is never called and SimulationContext leaks into the next parametrized test case.

The constructor call in test_direct_marl_env.py sits outside the try/finally block, so an init-time failure skips cleanup and can taint the subsequent CPU parametrize run. The kitless test in test_direct_marl_env_spaces.py calls a private method on a hand-assembled object, which is fragile if _configure_env_spaces reads any additional attribute initialised by init.

test_direct_marl_env.py — constructor/try/finally ordering; test_direct_marl_env_spaces.py — object.new + private-method approach.

Important Files Changed

Filename Overview
source/isaaclab/test/envs/test_direct_marl_env.py Refactors lifecycle test to use shared fixture and adds post-close assertions; constructor call is outside try/finally, risking a leaked SimulationContext if init raises.
source/isaaclab/test/envs/test_direct_marl_env_spaces.py New kitless unit test for agent counts and space construction; calls a private method on a manually-assembled object, which is fragile to internal refactors.
source/isaaclab/changelog.d/core-test-direct-marl.skip Changelog skip fragment for a test-only change; no functional concerns.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Test as test_initialization_and_close
    participant SU as sim_utils
    participant Env as DirectMARLEnv

    Test->>SU: create_new_stage()
    Test->>Env: "DirectMARLEnv(cfg=...) [OUTSIDE try/finally]"
    Note over Env: __init__ sets up SimulationContext,<br/>scene, spaces via _configure_env_spaces()
    Test->>Test: try block starts
    Test->>Env: assert not _is_closed
    Test->>SU: assert SimulationContext.instance() is env.sim
    Test->>Env: assert num_agents, spaces, state_space
    Test->>Test: finally block
    Test->>Env: env.close()
    Note over Env: clear_instance(), _is_closed = True
    Test->>Test: assert env._is_closed
    Test->>SU: assert SimulationContext.instance() is None
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Test as test_initialization_and_close
    participant SU as sim_utils
    participant Env as DirectMARLEnv

    Test->>SU: create_new_stage()
    Test->>Env: "DirectMARLEnv(cfg=...) [OUTSIDE try/finally]"
    Note over Env: __init__ sets up SimulationContext,<br/>scene, spaces via _configure_env_spaces()
    Test->>Test: try block starts
    Test->>Env: assert not _is_closed
    Test->>SU: assert SimulationContext.instance() is env.sim
    Test->>Env: assert num_agents, spaces, state_space
    Test->>Test: finally block
    Test->>Env: env.close()
    Note over Env: clear_instance(), _is_closed = True
    Test->>Test: assert env._is_closed
    Test->>SU: assert SimulationContext.instance() is None
Loading

Reviews (1): Last reviewed commit: "test: Split Direct MARL space coverage" | Re-trigger Greptile

Comment thread source/isaaclab/test/envs/test_direct_marl_env.py
Comment thread source/isaaclab/test/envs/test_direct_marl_env_spaces.py Outdated
@nvsekkin
nvsekkin requested a review from mataylor-nvidia July 16, 2026 22:22

@mataylor-nvidia mataylor-nvidia left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I have added a comment ontop of greptiles suggestion

nvsekkin and others added 5 commits July 16, 2026 23:00
Use an explicit test subclass to document which attributes space
configuration requires while keeping simulator initialization bypassed.
Replace the generic skip text with the test split rationale for the
review and release handoff.
@nvsekkin
nvsekkin requested a review from a team July 17, 2026 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants