Skip cloner perf test and fix ray caster intrinsics camera pose

[ooctipus]

Description

Two unrelated CI flakes in the same test infrastructure area, bundled because both are minimal targeted fixes.

1. test_disabled_fabric_change_notifies_speedup_regression (source/isaaclab_physx/test/sim/test_cloner.py)

This is a wall-clock perf regression test that's intended to run locally only — it asserts a >= 1.2× speedup of clone+reset with listener suspension. The result is platform-sensitive (deferred Fabric resync in sim.reset can offset the scene-time savings on some hardware), so it was meant to be skipped in CI.

The original guard was if os.getenv(\"CI\", \"\").lower() in (\"true\", \"1\"): pytest.skip(...) inside the test body. However, this project's CI doesn't set the CI env var — it selects tests via the isaacsim_ci pytest marker registered in pyproject.toml and applied module-wide via pytestmark at the top of test_cloner.py. Result: the env-var skip never fires, and the test runs (and occasionally flakes) on CI.

Fix: replace the dead env-var branch with a top-level @pytest.mark.skip(...) decorator so the test is collected and skipped unconditionally regardless of how CI selects tests. The correctness of the suspension mechanism is still covered by test_disabled_fabric_change_notifies_toggles_ifabricusd_flag, which is unaffected. Re-enable the perf test manually when touching listener suspension.

2. test_output_equal_to_usd_camera_when_intrinsics_set (source/isaaclab/test/sensors/test_ray_caster_camera.py)

Intermittent CI failure where the ray caster camera output mismatched the USD camera reference with inf-valued differences across all 518400 elements:

E   Mismatched elements: 518400 / 518400 (100.0%)
E   Greatest absolute difference: inf at index (0, 0, 0, 0) (up to 0.0001 allowed)
E   Greatest relative difference: inf at index (0, 0, 0, 0) (up to 0.005 allowed)

Root cause: the test placed the camera at eye=(0, 0, 5) looking at target=(0, 0, 0), which is colinear with the default up vector and produces a degenerate view transform. Fix: nudge eye to (0.001, 0, 5) for both the ray caster and USD camera, keeping them at identical poses while breaking the singularity. The underlying degeneracy is tracked in a separate internal ticket; this is the test-side mitigation.

Fixes # (n/a)

Type of change

• Bug fix (non-breaking change which fixes an issue)

Screenshots

N/A — test infrastructure changes.

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

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR replaces a dead CI environment variable check inside test_disabled_fabric_change_notifies_speedup_regression with an unconditional @pytest.mark.skip() decorator. The original skip logic never fired because Isaac Lab's CI doesn't set CI=true — it uses pytest markers for test selection. The change is correct and the approach is sound.

Architecture Impact

Self-contained. This only affects test infrastructure — no production code, no public APIs, no runtime behavior changes. The correctness of the listener suspension mechanism remains covered by test_disabled_fabric_change_notifies_toggles_ifabricusd_flag, which is unaffected.

Implementation Verdict

Ship it — minor documentation suggestion below.

Test Coverage

The PR correctly identifies that:

1. The perf regression test was meant to be local-only but was running (and flaking) in CI
2. The correctness test (test_disabled_fabric_change_notifies_toggles_ifabricusd_flag) remains active and covers the functional behavior
3. The @pytest.mark.skip decorator is the appropriate mechanism for unconditional skipping

The change removes dead code (import os, the env var check) and replaces it with the correct pytest idiom.

CI Status

No CI checks available yet. Once CI runs, this should result in the test being collected and skipped rather than executed and potentially flaking.

Findings

🔵 Improvement: test_cloner.py:555-559 — Consider documenting re-enablement procedure

The skip reason mentions "Re-enable manually when touching listener suspension" but doesn't specify how. A developer unfamiliar with the codebase might not know to simply comment out the decorator. Consider adding a brief note:

@pytest.mark.skip(
    reason=(
        "Local-only perf regression; correctness is covered by"
        " test_disabled_fabric_change_notifies_toggles_ifabricusd_flag."
        " To re-enable: comment out this decorator and run with --no-skip."
    )
)

This is purely a convenience suggestion — the current implementation is functionally correct.

🔵 Improvement: test_cloner.py:579-580 — Dead code inside skipped test is fine but could be cleaner

The remaining pytest.skip("omni::fabric::IFabricUsd unavailable") at line 580 is now unreachable since the test is unconditionally skipped. This is harmless (the test body never executes), but if you want to be thorough, you could remove it. However, keeping it preserves the original skip logic for when someone re-enables the test locally, which is the intended use case. No action needed.

---

The change is minimal, correct, and solves the stated problem. The diff removes exactly what's needed (the dead env var check) and adds the right pytest idiom (decorator-level skip). The docstring update from "Skipped under CI=true" to "Skipped unconditionally" accurately reflects the new behavior.

[greptile-apps]

Greptile Summary

This PR fixes a broken CI-skip guard in test_disabled_fabric_change_notifies_speedup_regression by replacing a dead os.getenv("CI", "") runtime check (which never fired because this project's CI doesn't set that env var) with a top-level @pytest.mark.skip(...) decorator that unconditionally skips the test at collection time. The cleanup also removes the now-unused import os and updates the docstring accordingly.

Confidence Score: 5/5

Safe to merge — minimal, targeted test infrastructure fix with no production code changes.

The change is a one-file test infrastructure fix that replaces a demonstrably broken skip guard with a correct decorator. No logic, API, or production code is affected. The correctness of the underlying feature remains covered by the adjacent toggle test.

No files require special attention.

Important Files Changed

Filename	Overview
source/isaaclab_physx/test/sim/test_cloner.py	Replaces dead os.getenv("CI") skip guard with @pytest.mark.skip(...) decorator; removes unused import os; updates docstring. Change is correct and minimal.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[pytest collects test_cloner.py]

    A --> B[BEFORE this PR]
    B --> D[Test body entered]
    D --> E{os.getenv CI set?}
    E -- No - CI never sets it --> F[Skip NOT triggered]
    F --> G[Test runs on CI and flakes]
    E -- Yes --> H[pytest.skip called]

    A --> C[AFTER this PR]
    C --> I[pytest.mark.skip decorator present]
    I --> J[Test skipped at collection time]

Loading

_{Reviews (1): Last reviewed commit: "Skip local-only fabric notice perf regre..." | Re-trigger Greptile}

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

The new commit adds the fix for test_output_equal_to_usd_camera_when_intrinsics_set — nudging the camera eye position from (0, 0, 5) to (0.001, 0, 5) to avoid the degenerate view transform singularity described in the PR description. Both previous suggestions from my earlier review were acknowledged (documentation clarity is acceptable as-is, dead code inside skipped test is intentionally preserved for local re-enablement).

Implementation Verdict

Ship it — both fixes are minimal, targeted, and correct.

Findings

No new issues. The camera position fix is applied consistently to both the ray caster and USD camera, maintaining identical poses while breaking the look-at singularity. The 0.001 offset is small enough to not meaningfully affect test semantics while being sufficient to produce a well-defined view matrix.