Skip to content

Split observation-space unit and integration tests#6633

Open
nvsekkin wants to merge 5 commits into
isaac-sim:developfrom
nvsekkin:esekkin/core-observation-space-tests
Open

Split observation-space unit and integration tests#6633
nvsekkin wants to merge 5 commits into
isaac-sim:developfrom
nvsekkin:esekkin/core-observation-space-tests

Conversation

@nvsekkin

@nvsekkin nvsekkin commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Description

Split manager-based RL observation-space construction from task-backed lifecycle coverage:

  • exercise _configure_gym_env_spaces as a simulator-free unit contract with synthetic vector, matrix, and image terms
  • preserve all eight original CPU/CUDA task-backed cases for reset/step, RGB, depth, and ray-caster observations
  • keep the task-backed file as an explicit relocation handoff with the isaaclab_tasks import required for task registration

This is PR3 of the focused replacement series for #6526.

Type of change

  • Test refactor (no 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

Keep Gym space construction simulator-free while preserving the task-backed
CPU/CUDA reset, camera, and ray-caster integration paths.
@nvsekkin
nvsekkin requested a review from a team July 20, 2026 18:56
@github-actions github-actions Bot added the isaac-lab Related to Isaac Lab team label Jul 20, 2026
@greptile-apps

greptile-apps Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR splits the existing test_manager_based_rl_env_obs_spaces.py into a simulator-free unit test file and a separate task-backed integration test file. The unit tests exercise _configure_gym_env_spaces directly via a lightweight SimpleNamespace stub, while all eight original simulator-dependent cases (reset/step, RGB, depth, ray-caster) are preserved in the new _task_integration.py file as a temporary relocation handoff.

  • Unit test rewrite (test_manager_based_rl_env_obs_spaces.py): replaces AppLauncher + full env init with object.__new__ + attribute stubs, marked pytest.mark.unit; adds explicit shape assertions for batched observation spaces.
  • New integration file (test_manager_based_rl_env_obs_spaces_task_integration.py): mirrors the original eight parametrized cases verbatim; adds import isaaclab_tasks for task registration; stays in the core test tree as a handoff until task scenarios migrate to the task package.

Confidence Score: 4/5

This is a test-only refactor with no production code changes; the split is clean and the stub-based unit tests correctly reflect the actual _configure_gym_env_spaces attribute surface.

The unit-test rewrite loses the insertion-order assertion that was the original guard for issue #3133 (using set instead of list for the dict-key check). This weakens the regression safety net slightly but does not affect production behaviour.

Both test files are worth a quick read; the integration file carries a pre-existing function-name typo, and the unit file's membership-only assertion for test_non_concatenated_obs_groups_contain_all_terms could be tightened.

Important Files Changed

Filename Overview
source/isaaclab/test/envs/test_manager_based_rl_env_obs_spaces.py Rewritten as a kitless unit test; uses object.__new__ + SimpleNamespace stubs to call _configure_gym_env_spaces directly. Adds batched shape assertions. Minor: ordering assertion weakened from list to set comparison.
source/isaaclab/test/envs/test_manager_based_rl_env_obs_spaces_task_integration.py New file preserving all eight task-backed integration cases verbatim. Contains a pre-existing function name typo test_obs_space_follows_clip_contraint (missing 's' in constraint).
source/isaaclab/changelog.d/core-observation-space-tests.skip Changelog skip fragment for this test-only split; content is appropriate.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Original test file\ntest_manager_based_rl_env_obs_spaces.py] -->|split into| B[Unit test\ntest_manager_based_rl_env_obs_spaces.py]
    A -->|extracted to| C[Integration test\ntest_manager_based_rl_env_obs_spaces_task_integration.py]

    B --> D[object.__new__ + SimpleNamespace stubs\nNo simulator required]
    D --> E[_configure_gym_env_spaces\ncalled directly]
    E --> F[2 unit tests\npytest.mark.unit]

    C --> G[AppLauncher + isaaclab_tasks\nSimulator required]
    G --> H[ManagerBasedRLEnv init\nfull lifecycle]
    H --> I[8 integration tests\npytest.mark.integration\ncpu x cuda x RGB/Depth/RayCaster]

    style B fill:#d4edda
    style C fill:#fff3cd
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"}}}%%
flowchart TD
    A[Original test file\ntest_manager_based_rl_env_obs_spaces.py] -->|split into| B[Unit test\ntest_manager_based_rl_env_obs_spaces.py]
    A -->|extracted to| C[Integration test\ntest_manager_based_rl_env_obs_spaces_task_integration.py]

    B --> D[object.__new__ + SimpleNamespace stubs\nNo simulator required]
    D --> E[_configure_gym_env_spaces\ncalled directly]
    E --> F[2 unit tests\npytest.mark.unit]

    C --> G[AppLauncher + isaaclab_tasks\nSimulator required]
    G --> H[ManagerBasedRLEnv init\nfull lifecycle]
    H --> I[8 integration tests\npytest.mark.integration\ncpu x cuda x RGB/Depth/RayCaster]

    style B fill:#d4edda
    style C fill:#fff3cd
Loading

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

ids=["RGB", "Depth", "RayCaster"],
)
@pytest.mark.parametrize("device", ["cpu", "cuda"])
def test_obs_space_follows_clip_contraint(env_cfg_cls, presets, device):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 The function name preserves the pre-existing typo contraint instead of constraint. Since the unit-test counterpart (test_obs_space_follows_clip_constraint) already spells this correctly, fixing the integration test name now would keep both files consistent and avoid the typo persisting into whatever final home these tests land in.

Suggested change
def test_obs_space_follows_clip_contraint(env_cfg_cls, presets, device):
def test_obs_space_follows_clip_constraint(env_cfg_cls, presets, device):

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +54 to 56
"""Non-concatenated observation groups expose every term in the Dict space (issue #3133).

Before the fix, only the last term in each non-concatenated group would be present

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Ordering assertion silently dropped

The original test used list(policy_space.spaces) == expected_policy_terms to verify both membership and insertion order, which was meaningful because the original bug (issue #3133) caused only the last term to survive. The new unit test replaces this with a set comparison, so a future regression where terms appear in the wrong order would go undetected. Switching to an ordered check — even with synthetic term names — would keep the full regression guard intact.

nvsekkin added 2 commits July 20, 2026 13:10
Restore ordered term coverage, correct the integration test name, and explain
why the test-only change skips a package version bump.
Use a general kitless unit-test home that can accommodate future
ManagerBasedRLEnv coverage while keeping the integration timeout explicit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

infrastructure isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant