Added Autotest to ROS2 - #866
Conversation
# Description Added the Autotest functionality for the ros 2 updated simulation Fixes # (861 - Broken Automatic Test) ## Type of change Please delete options that are not relevant. - New feature (non-breaking change which adds functionality) - This change requires a documentation update ## Does this PR introduce a breaking change? Restores the tests from ROS1 e.g. is old functionality not usable anymore ## Most important changes - added test folder with run_test.py and index_dict.py -added launch_leaderboard.test.sh to start the test from inside the container # Checklist: - [ ] My code follows the style guidelines of this project - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] 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 (might be obsolete with CI later on) - [ ] New and existing unit tests pass locally with my changes (might be obsolete with CI later on)
WalkthroughAdds a CARLA leaderboard test pipeline: a new launcher shell script, a Python test runner that manages simulation/agent/routes/statistics, test threshold metadata, a dev helper function, and edits to route XML data. Changes
Sequence DiagramsequenceDiagram
participant Shell as launch_leaderboard.test.sh
participant Env as Environment (reset/source/venv)
participant Carla as CARLA Simulator
participant Runner as run_test.py (TestScenario)
participant Agent as Agent Module
participant Stats as StatisticsManager
Shell->>Env: reset_env / source leaderboard env / activate venv
Env-->>Shell: env ready
Shell->>Carla: wait_for_carla.py
Carla-->>Shell: online
Shell->>Runner: exec run_test.py (args)
Runner->>Runner: parse args / init StatisticsManager & RouteIndexer
Runner->>Carla: connect / set sync mode & traffic manager
Carla-->>Runner: world ready
loop per scenario
Runner->>Agent: dynamic import & initialize (optionally start ROS bridge)
Agent-->>Runner: agent ready
loop per route
Runner->>Runner: load route / setup sensors / start watchdog
Runner->>Carla: start simulation for route
Agent->>Carla: control actions
Carla-->>Runner: simulation events / sensor data
Runner->>Stats: record route stats
end
Runner->>Runner: cleanup agent / unregister stats / reset world
end
Runner->>Shell: exit status / results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/leaderboard_launcher/scripts/launch_leaderboard.test.sh(1 hunks)code/test/index_dict.py(1 hunks)code/test/run_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
code/test/run_test.py (1)
code/leaderboard_launcher/leaderboard_launcher/paf_agent_base.py (1)
sensors(17-89)
code/leaderboard_launcher/scripts/launch_leaderboard.test.sh (1)
build/docker/agent-ros2/scripts/reset_env.bash (1)
reset_env(5-20)
🪛 Ruff (0.14.4)
code/test/run_test.py
1-1: Shebang is present but file is not executable
(EXE001)
167-167: Do not catch blind exception: Exception
(BLE001)
257-260: Create your own exception
(TRY002)
305-305: Do not catch blind exception: Exception
(BLE001)
321-321: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
326-326: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
368-368: Do not catch blind exception: Exception
(BLE001)
404-404: Do not catch blind exception: Exception
(BLE001)
421-421: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and push image
…ginning-of-autotest-routes hotfixed the autotest ater much research
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/routes/test.xml (1)
1770-1775: Remove stray waypoint element from scenario block.Line 1773 contains a misplaced
<position>element inside theInvadingTurn_4scenario definition:<offset value="0.55"/> <position x="355.1" y="6138.1" z="358"/> </scenario>Waypoint elements belong in the
<waypoints>section, not within<scenarios>. This breaks XML hierarchy and will cause parsing failures.<scenario name="InvadingTurn_4" type="InvadingTurn"> <trigger_point x="-557.5" y="4834.9" z="375.5" yaw="-179.9"/> <distance value="120"/> - <offset value="0.55"/><position x="355.1" y="6138.1" z="358"/> + <offset value="0.55"/> </scenario>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
build/docker/agent-ros2/scripts/devfunctions.bash(2 hunks)code/leaderboard_launcher/scripts/launch_leaderboard.test.sh(1 hunks)code/routes/routes_open_door.xml(1 hunks)code/routes/test.xml(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/leaderboard_launcher/scripts/launch_leaderboard.test.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-20T13:27:46.717Z
Learnt from: asamluka
Repo: una-auxme/paf PR: 624
File: code/routes/routes_bicycle.xml:55-57
Timestamp: 2025-01-20T13:27:46.717Z
Learning: In route configuration XMLs, scenarios (including ControlLoss) can be intentionally placed close together to test specific behaviors. There is no minimum spacing requirement between trigger points, as the spacing is determined by testing needs rather than distance rules.
Applied to files:
code/routes/routes_open_door.xmlcode/routes/test.xml
🔇 Additional comments (3)
build/docker/agent-ros2/scripts/devfunctions.bash (2)
16-18: Help text update is clear and consistent.The documentation for the new
leaderboard.testfunction is well-integrated into the existing help text, following the established format and tone.
67-72: New helper function is well-implemented.The
leaderboard.test()function follows the established patterns fromleaderboard.dev()andagent.dev():
- Proper subshell wrapping to isolate execution environment
- Correct export syntax for interactive shell availability
- Clear intent and minimal complexity
Please verify that the launcher script path
/workspace/code/leaderboard_launcher/scripts/launch_leaderboard.test.shis correct and accessible in the Docker build context.code/routes/test.xml (1)
1819-1819: The XML file is well-formed and parses successfully. Line 1819 contains a blank line (indentation) immediately after the<scenarios>opening tag, which is valid XML formatting. This pattern appears in at least two locations in the file and does not indicate incomplete edits or merge conflicts.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
build/docker/agent-ros2/scripts/devfunctions.bash(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
build/docker/agent-ros2/scripts/devfunctions.bash (2)
build/docker/agent-ros2/scripts/entrypoint-dev.sh (1)
_term(6-10)build/docker/agent-ros2/scripts/entrypoint-deploy.sh (1)
_term(4-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and push image
🔇 Additional comments (1)
build/docker/agent-ros2/scripts/devfunctions.bash (1)
71-76: Function implementation looks good.The new
leaderboard.test()function follows the established patterns in the file: it wraps the script invocation in a subshell, is properly exported for interactive use, and mirrors the structure of the existingleaderboard.dev()function. The help text is clear (once the path is corrected).
| - leaderboard.test: Launches the Simulation test at /code/leaderboard_launcher/scripts/launch_leaderboard.test.sh | ||
| Kill Terminal to end |
There was a problem hiding this comment.
Fix path discrepancy in help text.
The help text on line 16 references /code/leaderboard_launcher/scripts/launch_leaderboard.test.sh, but the function on line 73 uses /workspace/code/leaderboard_launcher/scripts/launch_leaderboard.test.sh. Align the documentation with the actual implementation.
Apply this diff to correct the help text:
- leaderboard.test: Launches the Simulation test at /code/leaderboard_launcher/scripts/launch_leaderboard.test.sh
+ leaderboard.test: Launches the Simulation test at /workspace/code/leaderboard_launcher/scripts/launch_leaderboard.test.shAlso applies to: 71-76
🤖 Prompt for AI Agents
In build/docker/agent-ros2/scripts/devfunctions.bash around lines 16-17 (and
also ensure consistency in lines 71-76), the help text references
/code/leaderboard_launcher/scripts/launch_leaderboard.test.sh but the function
actually calls
/workspace/code/leaderboard_launcher/scripts/launch_leaderboard.test.sh; update
the help/documentation string to use the /workspace/... path so it matches the
implementation (or alternatively change the function to /code/... if that path
is the desired canonical location) and ensure both the short help entry and the
longer description at lines 71-76 are changed consistently.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
code/test/run_test.py (2)
182-185: Guard sensor teardown against missing world.Line 182 calls
self.world.get_actors()without verifyingself.worldexists. If_load_and_wait_for_world()fails before assigningself.world, cleanup will raiseAttributeErrorand mask the original simulation failure.Apply this diff:
# Make sure no sensors are left streaming - alive_sensors = self.world.get_actors().filter("*sensor*") - for sensor in alive_sensors: - sensor.stop() - sensor.destroy() + if self.world: + alive_sensors = self.world.get_actors().filter("*sensor*") + for sensor in alive_sensors: + sensor.stop() + sensor.destroy()
580-644: Make threshold lookup resilient to varying route counts.Using
enumerate(route_records)to indexTEST_THRESHOLD_AND_NAMEcausesIndexErrorwhenroute_recordscontains more entries than the constant list (e.g., when routes are added toroutes/test.xml). Key thresholds by route ID instead.Consider this refactor:
route_records = test_evaluator.statistics_manager._results.checkpoint.records flags = [] - for i, route_record in enumerate(route_records): + + # Build a lookup: assuming TEST_THRESHOLD_AND_NAME has entries like (route_id, name, threshold) + # Adjust based on actual structure of TEST_THRESHOLD_AND_NAME + thresholds = { + entry[0]: (entry[1], entry[2]) + for entry in TEST_THRESHOLD_AND_NAME + } + + for route_record in route_records: flag = False - print(Fore.BLUE + f"Test: {TEST_THRESHOLD_AND_NAME[i][1]}") + route_id = route_record.index # or route_record.route_id, check actual attribute + route_name, time_limit = thresholds.get( + route_id, (f"Route {route_id}", None) + ) + + print(Fore.BLUE + f"Test: {route_name}") print(Fore.CYAN + "TIME TEST:") - if route_record.meta["duration_game"] > TEST_THRESHOLD_AND_NAME[i][2]: - print(f"should be in {TEST_THRESHOLD_AND_NAME[i][2]}") + if time_limit is not None and route_record.meta["duration_game"] > time_limit: + print(f"should be in {time_limit}") print(f"needed: {route_record.meta['duration_game']}") print(Fore.RED + "FAIL") flag = True - else: + elif time_limit is not None: print(Fore.GREEN + "SUCCEED")Verify the actual structure of
TEST_THRESHOLD_AND_NAMEandroute_recordattributes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/test/run_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
code/test/run_test.py (1)
code/leaderboard_launcher/leaderboard_launcher/paf_agent_base.py (1)
sensors(17-89)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and push image
🔇 Additional comments (2)
code/test/run_test.py (2)
211-228: LGTM!The world settings reset is properly guarded and correctly restores asynchronous mode after tests complete.
654-655: LGTM!Standard entry point pattern is correctly implemented.
| import traceback | ||
| import argparse | ||
| from argparse import RawTextHelpFormatter | ||
| from distutils.version import LooseVersion |
There was a problem hiding this comment.
Replace deprecated distutils with packaging.
distutils was deprecated in Python 3.10 and removed in Python 3.12, causing ImportError on newer Python versions.
Apply this diff:
-from distutils.version import LooseVersion
+from packaging.version import VersionThen update line 94:
- if LooseVersion(dist.version) < LooseVersion("0.9.10"):
+ if Version(dist.version) < Version("0.9.10"):Ensure packaging is listed in code/requirements.txt.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In code/test/run_test.py at line 20, replace the deprecated import "from
distutils.version import LooseVersion" with the equivalent from packaging by
importing "from packaging.version import Version" (or LooseVersion equivalent
usage) and update the code at line 94 to use packaging.Version (or the packaging
comparison API) instead of LooseVersion; also add "packaging" to
code/requirements.txt so the dependency is installed. Ensure any comparisons or
API calls are adapted to packaging.version semantics (e.g., Version(...)) and
run tests to verify behavior.
| if args.timeout: | ||
| client_timeout = args.timeout | ||
| client.set_timeout(client_timeout) |
There was a problem hiding this comment.
Fix undefined variable when timeout is not provided.
If args.timeout is falsy (0, None, False, empty string), client_timeout remains undefined at line 195, causing a NameError.
Apply this diff:
client = carla.Client(args.host, args.port)
+ client_timeout = self.client_timeout
if args.timeout:
client_timeout = args.timeout
client.set_timeout(client_timeout)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if args.timeout: | |
| client_timeout = args.timeout | |
| client.set_timeout(client_timeout) | |
| client = carla.Client(args.host, args.port) | |
| client_timeout = self.client_timeout | |
| if args.timeout: | |
| client_timeout = args.timeout | |
| client.set_timeout(client_timeout) |
🤖 Prompt for AI Agents
In code/test/run_test.py around lines 193-195, client_timeout can be undefined
when args.timeout is falsy; initialize it before the conditional and use an
explicit presence check. For example, set client_timeout = args.timeout (or
client_timeout = None) before the if, and change the conditional to if
args.timeout is not None: client_timeout = args.timeout so client_timeout is
always defined before client.set_timeout(client_timeout).
| self.sensor_icons = [ | ||
| sensors_to_icons[sensor["type"]] for sensor in self.sensors | ||
| ] |
There was a problem hiding this comment.
Handle missing sensor types in icon mapping.
Line 347 assumes all sensor types from self.sensors exist in sensors_to_icons. If an agent returns an unsupported sensor type, this raises KeyError and crashes the test without a clear error message.
Apply this diff:
self.sensor_icons = [
- sensors_to_icons[sensor["type"]] for sensor in self.sensors
+ sensors_to_icons.get(sensor["type"], "unknown_sensor")
+ for sensor in self.sensors
]Alternatively, validate sensor types earlier or provide a clearer error message.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.sensor_icons = [ | |
| sensors_to_icons[sensor["type"]] for sensor in self.sensors | |
| ] | |
| self.sensor_icons = [ | |
| sensors_to_icons.get(sensor["type"], "unknown_sensor") | |
| for sensor in self.sensors | |
| ] |
🤖 Prompt for AI Agents
In code/test/run_test.py around lines 346 to 348, the list comprehension assumes
every sensor["type"] exists in sensors_to_icons which can raise KeyError for
unsupported types; replace this with an explicit check per sensor: for each
sensor, if sensor["type"] is in sensors_to_icons use that mapping, otherwise
either assign a clear fallback icon or raise a descriptive error (e.g. include
the unknown type and allowed types) so tests fail with a helpful message;
alternatively move this validation earlier when sensors are ingested and
validate all types up front and fail fast with the same descriptive error.
| "--resume", | ||
| type=bool, | ||
| default=False, | ||
| help="Resume execution from last checkpoint?", |
There was a problem hiding this comment.
Fix incorrect boolean argument parsing.
Using type=bool with argparse converts any non-empty string to True, making --resume False still resolve to True. Use action='store_true' instead.
Apply this diff:
parser.add_argument(
"--resume",
- type=bool,
- default=False,
+ action='store_true',
help="Resume execution from last checkpoint?",
)🤖 Prompt for AI Agents
In code/test/run_test.py around lines 552 to 555, the argparse option is using
type=bool which treats any non-empty string as True; replace the argument
definition to use action='store_true' (and remove the type and default
parameters) so that passing --resume sets the flag True and omitting it leaves
it False; keep the help text unchanged.
2 participants
Description
Added the Autotest functionality for the ros 2 updated simulation
Fixes # (861 - Broken Automatic Test)
Type of change
Please delete options that are not relevant.
Does this PR introduce a breaking change?
Restores the tests from ROS1
e.g. is old functionality not usable anymore
Most important changes
-added launch_leaderboard.test.sh to start the test from inside the container
Checklist:
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.