Speed up environment smoke tests and cover EventManager interval handlers#5713
Conversation
There was a problem hiding this comment.
🔍 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_stepsfrom 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 (likepush_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 ofis_global_timesetting.
3. Clean Code Hygiene
- Removing the stale
@pytest.mark.xfaildecorator fromtest_environments_newton.pyis 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.skipfor the test-only changesesekkin-pr-a-newton-xfail.rstfor 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:
continueConsider 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 SummaryThis PR speeds up environment smoke tests by reducing the default step count from 100 to 20, and adds a
Confidence Score: 4/5Safe 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
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"]
|
| for term_name in dir(events_cfg): | ||
| if term_name.startswith("_"): | ||
| continue | ||
| try: | ||
| term = getattr(events_cfg, term_name, None) | ||
| except AttributeError: | ||
| continue | ||
| if ( | ||
| term is not None | ||
| and getattr(term, "mode", None) == "interval" | ||
| and getattr(term, "interval_range_s", None) is not None | ||
| ): | ||
| term.interval_range_s = (1e-6, 1e-6) |
There was a problem hiding this comment.
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.
…lers (isaac-sim#5713) # 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. ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] 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 - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there
…lers (isaac-sim#5713) # 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. ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] 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 - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there
…lers (isaac-sim#5713) # 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. ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] 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 - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there
Description
num_stepsfrom 100 to 20 in_run_environments._force_interval_events_to_fire_immediately()and opt-inforce_interval_eventson the test helpers. If there is nointervalmode, this function is a no-op. For all others, this helps test theintervalcritical path at eachstep()call.Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there