Skip to content

feat: add additional package args to ROS test #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
MrBlenny wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat: add additional package args to ROS test #12

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 26 additions & 9 deletions platform_cli/groups/ros.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ def command():
command()

@ros.command(name="test")
@click.option("--package", type=str, default=None, help="The package to test")
@click.option("--package", "-p", type=str, multiple=True, help="The package(s) to test")
@click.option("--results-dir", type=Path, default=None)
@click.option(
"--watch", type=bool, is_flag=True, default=False, help="Should we watch for changes?"
Expand All @@ -101,30 +101,46 @@ def command():
default=2,
help="Number of times to retest until pass",
)
@click.option(
"--include-dependents",
type=bool,
is_flag=True,
default=False,
help="Also test packages that depend on changed packages (used with --changed)",
)
Comment on lines +104 to +110
Copy link

@coderabbitai coderabbitai bot Sep 29, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Dependents vs dependencies: --packages-up-to is the wrong selector for “dependents”.

--packages-up-to selects the package(s) and their dependencies. To include packages that depend on the selected ones, use --packages-above or --packages-above-and-dependencies. Current code will test dependencies, not dependents. This is a logic bug that will miss intended targets.

Apply this diff:

-        @click.option(
-            "--include-dependents",
+        @click.option(
+            "--include-dependents",
             type=bool,
             is_flag=True,
             default=False,
-            help="Also test packages that depend on changed packages (used with --changed)",
+            help="Also test packages that depend on the selected packages",
         )
-            if packages_to_test:
-                if include_dependents:
-                    args_str_test += f" --packages-up-to {packages_str}"
-                else:
-                    args_str_test += f" --packages-select {packages_str}"
+            if packages_to_test:
+                if include_dependents:
+                    args_str_test += f" --packages-above-and-dependencies {packages_str}"
+                else:
+                    args_str_test += f" --packages-select {packages_str}"

Please confirm the intended semantics and that --changed is implemented elsewhere (your help text references it but it’s not defined here). Run:

Also verify colcon selectors:

Also applies to: 130-134

🏁 Script executed:

#!/bin/bash
# Check for --changed usage in the repo
rg -nP --type=python -C2 '\-\-changed'

Length of output: 30

🌐 Web query:

In colcon, what do --packages-select, --packages-up-to, --packages-above, and --packages-above-and-dependencies select?

💡 Result:

  • --packages-select <pkg...>: selects only the named package(s). [1]

  • --packages-up-to <pkg...>: selects the named package(s) and all their recursive dependencies (i.e., packages they depend on). [1]

  • --packages-above <pkg...>: selects the named package(s) and all packages that (recursively) depend on them (i.e., their dependents). [1]

  • --packages-above-and-dependencies <pkg...>: selects the named package(s), all packages that depend on them, and those dependents' recursive dependencies. (Equivalent to union of --packages-above and their dependencies.) [1]

Reference: colcon package selection arguments. [1] https://colcon.readthedocs.io/en/main/reference/package-selection-arguments.html

🏁 Script executed:

#!/bin/bash
# Show context around the click.option snippet in ros.py
fd ros.py -x sed -n '1,200p' {}

# Search for --changed flag across all Python files
rg -g "*.py" -n "\-\-changed" -C2

Length of output: 7827

Fix dependents logic in ros test
The --include-dependents flag currently:

  • Uses --packages-up-to, which selects dependencies rather than dependents.
  • References a non-existent --changed option in its help text.

Apply:

  • Change help to help="Also test packages that depend on the selected packages".
  • In the test command’s selector logic, replace --packages-up-to with --packages-above-and-dependencies when include_dependents is true.
🤖 Prompt for AI Agents 
In platform_cli/groups/ros.py around lines 104 to 110, update the option help
and the selector logic: change the help string for --include-dependents to "Also
test packages that depend on the selected packages" (removing the incorrect
reference to --changed), and in the test command selector code branch where
include_dependents is True, replace use of the --packages-up-to selector flag
with --packages-above-and-dependencies so the command selects dependents (above)
and their dependencies instead of only dependencies.

@click.argument("args", nargs=-1)
def test(
package: str,
package: List[str],
results_dir: Path,
watch: bool,
build: bool,
retest_until_pass: int,
include_dependents: bool,
args: List[str],
):
"""Runs colcon test on all ROS packages"""

env = get_ros_env()
args_str = " ".join(args)

# Some args only apply to colcon test
args_str_test = args_str
args_str_build = args_str
if package:
args_str_test += f" --packages-select {package}"
args_str_build += f" --package {package}"

packages_to_test = list(package) if package else []
packages_str = " ".join(packages_to_test)

if packages_to_test:
if include_dependents:
args_str_test += f" --packages-up-to {packages_str}"
else:
args_str_test += f" --packages-select {packages_str}"

def command():
if build:
args_str_build = args_str
if packages_to_test:
for pkg in packages_to_test:
args_str_build += f" --package {pkg}"
call(" ".join(["platform ros build", args_str_build]))
Comment on lines +138 to 142
Copy link

@coderabbitai coderabbitai bot Sep 29, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Build step only builds the last package; construct a single colcon build with proper selection.

Passing --package repeatedly to platform ros build (which accepts a single --package) means Click will take the last value. Also, when include_dependents is set we should mirror the same selection here.

Apply this diff to build in-place (avoids re-invoking our CLI and ensures correct selection):

-                if build:
-                    args_str_build = args_str
-                    if packages_to_test:
-                        for pkg in packages_to_test:
-                            args_str_build += f" --package {pkg}"
-                    call(" ".join(["platform ros build", args_str_build]))
+                if build:
+                    args_str_build = args_str
+                    if packages_to_test:
+                        packages_str = " ".join(packages_to_test)
+                        if include_dependents:
+                            args_str_build += f" --packages-above-and-dependencies {packages_str}"
+                        else:
+                            args_str_build += f" --packages-select {packages_str}"
+                    call(
+                        " ".join(
+                            [
+                                "colcon build",
+                                "--merge-install",
+                                "--symlink-install",
+                                f"--install-base /opt/greenroom/{env['PLATFORM_MODULE']}",
+                                args_str_build,
+                            ]
+                        )
+                    )

Alternatively, update platform ros build to accept multiple=True on --package and apply a single --packages-select with the joined list.

📝 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.

Suggested change
args_str_build = args_str
if packages_to_test:
for pkg in packages_to_test:
args_str_build += f" --package {pkg}"
call(" ".join(["platform ros build", args_str_build]))
if build:
args_str_build = args_str
if packages_to_test:
packages_str = " ".join(packages_to_test)
if include_dependents:
args_str_build += f" --packages-above-and-dependencies {packages_str}"
else:
args_str_build += f" --packages-select {packages_str}"
call(
" ".join(
[
"colcon build",
"--merge-install",
"--symlink-install",
f"--install-base /opt/greenroom/{env['PLATFORM_MODULE']}",
args_str_build,
]
)
)
🤖 Prompt for AI Agents 
In platform_cli/groups/ros.py around lines 138-142, the current loop appends
multiple "--package" flags which Click accepts only once so only the last
package is built; replace this by constructing a single in-place colcon build
invocation that uses "--packages-select" with a comma/space-joined list of
packages (and include dependents when include_dependents is set), or
alternatively change the platform ros build command to accept multiple=True for
the "--package" option and internally join them into one "--packages-select"
argument; ensure you call the colcon build directly (build-in-place) rather than
re-invoking the CLI so all selected packages are built correctly.


p1 = call(
" ".join(
[
Expand All @@ -148,7 +164,8 @@ def command():
p1, p2 = command()

if results_dir and results_dir.exists():
collect_xunit_xmls(results_dir, package)
for pkg in packages_to_test:
collect_xunit_xmls(results_dir, pkg)

exit(max([p1.returncode, p2.returncode]))

Expand Down