Speed up environment smoke tests and cover EventManager interval handlers

[nvsekkin]

Description

• Reduce the default num_steps from 100 to 20 in _run_environments.
• Add _force_interval_events_to_fire_immediately() and opt-in force_interval_events on the test helpers. If there is no interval mode, this function is a no-op. For all others, this helps test the interval critical path at each step() call.
• This PR opts in test_environments.py only. Follow-up Force interval events by default #5739 flips the default to True.

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]

🔍 Automated Code Review

Summary

This PR improves test efficiency and coverage for environment smoke tests. The changes reduce default test steps from 100 to 20, add a mechanism to force interval-mode event handlers to fire immediately, and remove a stale xfail decorator from Newton tests.

---

✅ Strengths

1. Test Performance Optimization

• Reducing num_steps from 100 to 20 (80% reduction) should significantly speed up CI smoke tests while maintaining coverage for basic functionality validation.

2. Improved Test Coverage for EventManager

• The _force_interval_events_to_fire_immediately() helper elegantly solves the problem of interval handlers (like push_robot) never firing within short test budgets.
• Setting interval_range_s = (1e-6, 1e-6) guarantees immediate triggering on the first step - this is a clever approach that works regardless of is_global_time setting.

3. Clean Code Hygiene

• Removing the stale @pytest.mark.xfail decorator from test_environments_newton.py is good housekeeping since the underlying Hydra deep-nesting issue was resolved in PRs #5029, #5130, and #5177.

4. Well-Documented Changes

• The changelog fragments clearly explain both changes:
  • esekkin-force-interval-events.skip for the test-only changes
  • esekkin-pr-a-newton-xfail.rst for the xfail removal fix

---

🔶 Suggestions

1. Consider Defensive Type Checking
In _force_interval_events_to_fire_immediately(), the code catches AttributeError but could be more defensive:

# Current
try:
    term = getattr(events_cfg, term_name, None)
except AttributeError:
    continue

Consider adding a check for callable attributes or descriptor edge cases that might raise other exceptions.

2. Documentation Consistency
The force_interval_events parameter is documented in _check_random_actions() but only the single-agent path applies it. Consider if multi-agent environments should also support this parameter (currently the flag is passed but the helper may need verification for multi-agent event configs).

3. Magic Number Documentation
The value 1e-6 appears both in the helper and matches the EventManager threshold. Consider adding a comment referencing where this threshold is defined in the main codebase to help future maintainers.

---

📋 Architecture & Design

The approach is sound:

• Uses config mutation rather than runtime patching, keeping test isolation clean
• The helper function is idempotent (safe to call multiple times)
• No changes to production code - purely test infrastructure improvements

---

🧪 Test Coverage Assessment

Aspect	Status
Interval event triggering	✅ Now covered
Newton preset environments	✅ Enabled (xfail removed)
Step budget efficiency	✅ Improved (100→20 steps)
Backward compatibility	✅ Preserved (force_interval_events defaults to False)

---

📝 Update (10da66d)

Reviewed merge commit bringing in upstream develop changes. No changes to the PR's own code — the 4 files (env_test_utils.py, test_environments.py, and the two changelog fragments) remain unchanged from the previous review.

---

This is an automated review. Please verify all suggestions against project conventions.

[greptile-apps]

Greptile Summary

This PR speeds up environment smoke tests by reducing the default step count from 100 to 20, and adds a force_interval_events flag that rewrites interval-mode EventTermCfg bounds to (1e-6, 1e-6) before environment creation so that handlers like push_robot always fire on the first step. It also removes the stale @pytest.mark.xfail from test_environments_newton whose underlying Hydra deep-nesting issue was resolved in PR #5029.

• env_test_utils.py: Introduces _force_interval_events_to_fire_immediately (iterates public attributes of env_cfg.events via dir(), sets matching interval bounds to 1e-6) and threads a new force_interval_events parameter through _run_environments / _check_random_actions; default num_steps drops from 100 → 20.
• test_environments.py: Opts in to force_interval_events=True so all standard CI tasks now exercise interval handlers within the 20-step budget.
• test_environments_newton.py: Removes the @pytest.mark.xfail decorator; Newton CI tests are now expected to pass unconditionally.

Confidence Score: 4/5

Safe to merge; changes are test-only and the core logic of the interval-forcing helper is correctly aligned with EventManager's sampling and trigger behaviour.

The interval-forcing math is correct: setting both bounds to 1e-6 guarantees time_left = 1e-6 after reset, which drops below the 1e-6 trigger threshold on the very first dt subtraction. The xfail removal is adequately justified by the referenced PRs. Two minor gaps exist: the assignment in _force_interval_events_to_fire_immediately is unguarded against frozen dataclass configs, and the Newton test still won't exercise interval handlers within its 20-step budget.

env_test_utils.py (unguarded attribute assignment in the interval-forcing helper) and test_environments_newton.py (missing force_interval_events=True).

Important Files Changed

Filename	Overview
source/isaaclab_tasks/test/env_test_utils.py	Core change: adds _force_interval_events_to_fire_immediately helper and force_interval_events parameter; reduces default num_steps from 100 to 20. Logic is sound but dir()-based iteration could silently skip frozen-dataclass terms.
source/isaaclab_tasks/test/test_environments.py	Passes force_interval_events=True so interval handlers (e.g. push_robot) now exercise on step 1 instead of never within the 20-step budget.
source/isaaclab_tasks/test/test_environments_newton.py	Removes stale @pytest.mark.xfail (issue resolved in PR #5029); does not add force_interval_events=True, so Newton tests still skip interval handler coverage.
source/isaaclab_tasks/changelog.d/esekkin-force-interval-events.skip	Changelog skip marker for the test-only change; content matches actual diff.
source/isaaclab_tasks/changelog.d/esekkin-pr-a-newton-xfail.rst	Changelog entry documenting removal of the xfail marker; accurate description.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["test_environments / test_environments_newton\ncalls _run_environments(force_interval_events=True/False)"] --> B["_run_environments"]
    B --> C["_check_random_actions(force_interval_events=...)"]
    C --> D["parse_env_cfg → env_cfg"]
    D --> E{physics_preset_name?}
    E -- yes --> F["apply_overrides (physics preset)"]
    F --> G{force_interval_events?}
    E -- no --> G
    G -- yes --> H["_force_interval_events_to_fire_immediately(env_cfg)\nsets interval_range_s = (1e-6, 1e-6) for all interval terms"]
    G -- no --> I["gym.make(task_name, cfg=env_cfg)"]
    H --> I
    I --> J["EventManager.__init__\nsamples time_left from interval_range_s → 1e-6"]
    J --> K["env.reset()"]
    K --> L["step loop (num_steps=20)"]
    L --> M["EventManager.apply('interval', dt=dt)\ntime_left -= dt → fires on step 1"]
    M --> N{"check valid tensors"}
    N -- pass --> L
    N -- fail --> O["test FAIL"]
    L -- done --> P["test PASS"]

Loading

Comments Outside Diff (1)

1. source/isaaclab_tasks/test/test_environments_newton.py, line 39-41 (link)

   Newton smoke tests don't exercise interval handlers

   test_environments_newton doesn't pass force_interval_events=True. With the new 20-step budget and typical interval ranges of several seconds, interval-mode event terms (e.g. push_robot) will never fire during Newton CI runs — the same gap this PR fixes for the standard test_environments. Consider passing force_interval_events=True here as well, unless the Newton preset's physics interactions make forcing interval events undesirable.

_{Reviews (1): Last reviewed commit: "Merge branch 'develop' into esekkin/forc..." | Re-trigger Greptile}

[greptile-apps]

dir() iteration may skip frozen-dataclass configs silently

dir(events_cfg) returns all public names — including callable methods and descriptors. The try/except AttributeError guards against runtime property errors, but the subsequent assignment term.interval_range_s = (1e-6, 1e-6) (line 250) is unguarded. If any EventTermCfg subclass is declared with frozen=True (or uses __setattr__ guards), the assignment raises FrozenInstanceError (a subclass of TypeError) and propagates uncaught, aborting the test with a confusing traceback rather than a helpful assertion failure. Wrapping the assignment in a try/except (AttributeError, TypeError) block would make the helper resilient to these cases.