Skip to content

Fix test fail associated with bad merge. - #995

Merged
alexmillane merged 1 commit into
mainfrom
alex/fix/incorrect_import_from_bad_merge
Jul 31, 2026
Merged

Fix test fail associated with bad merge.#995
alexmillane merged 1 commit into
mainfrom
alex/fix/incorrect_import_from_bad_merge

Conversation

@alexmillane

@alexmillane alexmillane commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Summary

FIx an import error on test startup

Detailed description

@greptile-apps

greptile-apps Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Updates the environment runner mouse-interaction test to use the available persistent SimulationApp helper instead of the invalid subprocess helper import.

Confidence Score: 5/5

The PR appears safe to merge.

The replacement helper accepts the test callable and headless argument, supplies the expected SimulationApp parameter, and propagates the callable's boolean result for the existing assertion.

Important Files Changed

Filename Overview
isaaclab_arena/tests/test_environment_runner.py Replaces the broken helper import and updates its sole invocation with a signature-compatible persistent SimulationApp helper.

Reviews (1): Last reviewed commit: "Fix test fail associated with bad merge." | Re-trigger Greptile

@arena-review-bot

Copy link
Copy Markdown
Contributor

🤖 Isaac Lab-Arena Review Bot

Summary

This PR fixes a broken import in test_environment_runner.py left behind by a bad merge: run_simulation_app_function was imported from isaaclab_arena.tests.utils.subprocess, but that module only defines run_subprocess — so test collection failed. The fix swaps to run_function_with_persistent_simulation_app from tests.utils.persistent_simulation_app, whose signature (function, headless=True) matches the updated call site. Verified the old symbol is absent from subprocess.py and the new one exists and lines up.

The inner/outer simulation-test pattern is intact (deferred carb/omni imports in the inner function), and this in-process persistent-app test correctly stays marker-free for Phase 1. No further concerns.

Verdict

Ship it

@cvolkcvolk cvolkcvolk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

@cvolkcvolk
cvolkcvolk enabled auto-merge (squash) July 31, 2026 14:52
@alexmillane
alexmillane disabled auto-merge July 31, 2026 14:56
@alexmillane
alexmillane merged commit eed71d5 into main Jul 31, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants