From 25513a156d40def498d2f4b3f5857cc94c74dbee Mon Sep 17 00:00:00 2001 From: Aljoscha Schmidt Date: Mon, 1 Sep 2025 12:36:44 +0200 Subject: [PATCH 1/5] Test Validator CLI --- package_xml_validation/helpers/workspace.py | 14 ++ .../package_xml_validator.py | 22 +-- tests/test_package_xml_formatted_cli.py | 175 ++++++++++++++++++ tests/test_workspace.py | 46 ++++- 4 files changed, 237 insertions(+), 20 deletions(-) create mode 100644 tests/test_package_xml_formatted_cli.py diff --git a/package_xml_validation/helpers/workspace.py b/package_xml_validation/helpers/workspace.py index 79c3f33..73ec061 100644 --- a/package_xml_validation/helpers/workspace.py +++ b/package_xml_validation/helpers/workspace.py @@ -128,6 +128,20 @@ def get_pkgs_in_wrs(path: Path) -> list[str]: return sorted(pkgs) +def find_package_xml_files(paths) -> list[str]: + files = [] + for p in paths: + p = Path(p) + if p.is_file() and p.name == "package.xml": + files.append(p) + elif p.is_file() and p.name == "CMakeLists.txt": + files.append(p.parent / "package.xml") + elif p.is_dir(): + for xml in p.rglob("package.xml"): + files.append(xml) + return sorted({str(x) for x in files}) + + def main() -> None: ap = argparse.ArgumentParser( description=( diff --git a/package_xml_validation/package_xml_validator.py b/package_xml_validation/package_xml_validator.py index c1914fa..dd756be 100644 --- a/package_xml_validation/package_xml_validator.py +++ b/package_xml_validation/package_xml_validator.py @@ -9,12 +9,14 @@ from .helpers.pkg_xml_formatter import PackageXmlFormatter from .helpers.cmake_parsers import read_deps_from_cmake_file from .helpers.find_launch_dependencies import scan_files + from .helpers.workspace import find_package_xml_files except ImportError: from helpers.logger import get_logger from helpers.rosdep_validator import RosdepValidator from helpers.pkg_xml_formatter import PackageXmlFormatter from helpers.cmake_parsers import read_deps_from_cmake_file from helpers.find_launch_dependencies import scan_files + from helpers.workspace import find_package_xml_files import subprocess import re @@ -67,24 +69,6 @@ def __init__( if self.compare_with_cmake: self.num_checks += 1 - def find_package_xml_files(self, paths): - """Locate all package.xml files within the provided paths.""" - package_xml_files = [] - for path in paths: - if os.path.isfile(path) and os.path.basename(path) == "package.xml": - package_xml_files.append(path) - elif os.path.isfile(path) and os.path.basename(path) == "CMakeLists.txt": - package_xml_files.append( - os.path.join(os.path.dirname(path), "package.xml") - ) - elif os.path.isdir(path): - for root, _, files in os.walk(path): - if "package.xml" in files: - package_xml_files.append(os.path.join(root, "package.xml")) - # Filter out duplicates - package_xml_files = list(set(package_xml_files)) - return package_xml_files - def get_package_type(self, xml_file: str) -> tuple[PackageType, bool]: """Determine the package type based on the presence of CMakeLists.txt or setup.py. Returns where bool indicates if the package is a message package.""" @@ -597,7 +581,7 @@ def check_and_format_files(self, package_xml_files): return True def check_and_format(self, src): - package_xml_files = self.find_package_xml_files(src) + package_xml_files = find_package_xml_files(src) if not package_xml_files: self.logger.info("No package.xml files found in the provided paths.") return diff --git a/tests/test_package_xml_formatted_cli.py b/tests/test_package_xml_formatted_cli.py new file mode 100644 index 0000000..94ae284 --- /dev/null +++ b/tests/test_package_xml_formatted_cli.py @@ -0,0 +1,175 @@ +# tests/test_package_xml_formatted_cli.py +import io +import os +import sys +import unittest +import tempfile +from pathlib import Path +from contextlib import redirect_stdout +from unittest import mock + +# SUT +from package_xml_validation import package_xml_validator as SUT + + +def make_fake_validator_factory( + *, + return_from_check_and_format=True, + return_from_check_and_format_files=True, + constructed_out=None, +): + """ + Build a fake PackageXmlValidator class to patch into SUT. + Captures constructor kwargs and method calls, and returns the values provided. + """ + constructed_out = constructed_out if constructed_out is not None else [] + + class FakeValidator: + def __init__(self, **kwargs): + self.init_kwargs = kwargs + self.check_and_format_calls = [] + self.check_and_format_files_calls = [] + self._ret_check_and_format = return_from_check_and_format + self._ret_check_and_format_files = return_from_check_and_format_files + constructed_out.append(self) + + def check_and_format(self, src): + self.check_and_format_calls.append(list(src)) + return self._ret_check_and_format + + def check_and_format_files(self, files): + self.check_and_format_files_calls.append(list(files)) + return self._ret_check_and_format_files + + return FakeValidator, constructed_out + + +class TestPackageXmlFormattedCLI(unittest.TestCase): + def setUp(self): + # Make a temp directory to act as predictable CWD or input paths + self.tmp = tempfile.TemporaryDirectory() + self.tmpdir = Path(self.tmp.name) + + def tearDown(self): + self.tmp.cleanup() + + def test_cli_defaults_uses_cwd_and_skips_rosdep_when_env_missing(self): + """No args -> src defaults to CWD; without ROS_DISTRO, rosdep check is skipped.""" + fake_cls, constructed = make_fake_validator_factory() + with mock.patch.object(SUT, "PackageXmlValidator", fake_cls): + with mock.patch.object(sys, "argv", ["prog"]): + with mock.patch.dict(os.environ, {}, clear=True): + with mock.patch.object( + SUT.os, "getcwd", return_value=str(self.tmpdir) + ): + buf = io.StringIO() + with redirect_stdout(buf): + SUT.main() + out = buf.getvalue() + + # One instance constructed + self.assertEqual(len(constructed), 1) + inst = constructed[0] + + # src defaults to CWD + self.assertEqual(inst.check_and_format_calls, [[str(self.tmpdir)]]) + + # With no ROS_DISTRO -> skip rosdep validation + self.assertIs(inst.init_kwargs["check_rosdeps"], False) + self.assertIn( + "ROS_DISTRO environment variable not set. Skipping rosdep key validation.", + out, + ) + + # When rosdep is skipped, compare_with_cmake should still be False by default + self.assertIs(inst.init_kwargs["compare_with_cmake"], False) + + def test_cli_file_option_calls_check_and_format_files(self): + """--file takes precedence over src; passes exact file to check_and_format_files.""" + xml = self.tmpdir / "pkg" / "package.xml" + xml.parent.mkdir(parents=True, exist_ok=True) + xml.write_text("demo", encoding="utf-8") + + fake_cls, constructed = make_fake_validator_factory() + with mock.patch.object(SUT, "PackageXmlValidator", fake_cls): + with mock.patch.object(sys, "argv", ["prog", "--file", str(xml)]): + # Provide ROS_DISTRO so rosdep check is enabled + with mock.patch.dict(os.environ, {"ROS_DISTRO": "jazzy"}, clear=True): + buf = io.StringIO() + with redirect_stdout(buf): + SUT.main() + + self.assertEqual(len(constructed), 1) + inst = constructed[0] + + # check_and_format_files called with exactly the file passed + self.assertEqual(inst.check_and_format_files_calls, [[str(xml)]]) + # path forwarded as the file + self.assertEqual(inst.init_kwargs["path"], str(xml)) + # rosdep validation enabled when ROS_DISTRO present + self.assertTrue(inst.init_kwargs["check_rosdeps"]) + + def test_cli_compare_with_cmake_disallowed_when_skip_rosdep(self): + """--compare-with-cmake is disabled if rosdep validation is skipped (no ROS_DISTRO).""" + fake_cls, constructed = make_fake_validator_factory() + with mock.patch.object(SUT, "PackageXmlValidator", fake_cls): + with mock.patch.object(sys, "argv", ["prog", "--compare-with-cmake"]): + with mock.patch.dict(os.environ, {}, clear=True): + buf = io.StringIO() + with redirect_stdout(buf): + SUT.main() + out = buf.getvalue() + + self.assertEqual(len(constructed), 1) + inst = constructed[0] + + # Must print the incompatibility message and disable the flag + self.assertIn( + "Cannot use --compare-with-cmake with --skip-rosdep-key-validation.", out + ) + self.assertFalse(inst.init_kwargs["check_rosdeps"]) + self.assertFalse(inst.init_kwargs["compare_with_cmake"]) + + def test_cli_verbose_and_xmllint_flags_propagate(self): + """--verbose and --check-with-xmllint should propagate to validator args.""" + fake_cls, constructed = make_fake_validator_factory() + with mock.patch.object(SUT, "PackageXmlValidator", fake_cls): + with mock.patch.object( + sys, "argv", ["prog", "--verbose", "--check-with-xmllint"] + ): + with mock.patch.dict(os.environ, {"ROS_DISTRO": "jazzy"}, clear=True): + buf = io.StringIO() + with redirect_stdout(buf): + SUT.main() + + self.assertEqual(len(constructed), 1) + inst = constructed[0] + self.assertTrue(inst.init_kwargs["verbose"]) + self.assertTrue(inst.init_kwargs["check_with_xmllint"]) + self.assertTrue(inst.init_kwargs["check_rosdeps"]) + + def test_cli_exit_code_on_failure(self): + """When validator returns False, CLI should exit(1).""" + # Fake returns False on both code paths + fake_cls, constructed = make_fake_validator_factory( + return_from_check_and_format=False, + return_from_check_and_format_files=False, + ) + with mock.patch.object(SUT, "PackageXmlValidator", fake_cls): + # Use --file to take the check_and_format_files path + xml = self.tmpdir / "p" / "package.xml" + xml.parent.mkdir(parents=True, exist_ok=True) + xml.write_text("x", encoding="utf-8") + + with mock.patch.object(sys, "argv", ["prog", "--file", str(xml)]): + with mock.patch.dict(os.environ, {"ROS_DISTRO": "jazzy"}, clear=True): + with self.assertRaises(SystemExit) as cm: + SUT.main() + self.assertEqual(cm.exception.code, 1) + self.assertEqual(len(constructed), 1) + # Sanity: we took the --file path + self.assertEqual(constructed[0].check_and_format_files_calls, [[str(xml)]]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_workspace.py b/tests/test_workspace.py index f31d93a..1265e13 100644 --- a/tests/test_workspace.py +++ b/tests/test_workspace.py @@ -42,6 +42,11 @@ def close(self): self._tmp.cleanup() +def _as_str_set(paths) -> set[str]: + """Normalize potentially mixed Path/str iterables to a set of strings.""" + return {str(p) for p in paths} + + class TestWorkspaceHelpers(unittest.TestCase): def setUp(self): self.t = TempTree() @@ -63,7 +68,7 @@ def setUp(self): # Ignored via parent directory COLCON_IGNORE third_party = src / "third_party" - third_party.mkdir(parents=True, exist_ok=True) # <-- ensure parent exists + third_party.mkdir(parents=True, exist_ok=True) # ensure parent exists (third_party / "COLCON_IGNORE").write_text("") self.pkg_third = self.t.pkg(third_party, "third_pkg", name_in_xml="third_pkg") @@ -165,6 +170,45 @@ def test_get_pkgs_in_wrs_nonexistent_string_raises_filenotfound(self): with self.assertRaises(FileNotFoundError): SUT.get_pkgs_in_wrs(bogus) + # --- find_package_xml_files -------------------------------------------- + def test_find_package_xml_files_mixed_inputs_and_dedup(self): + """Mix direct package.xml, CMakeLists.txt sibling, and a dir; expect de-dup.""" + cmake = self.t.touch( + self.pkg1 / "CMakeLists.txt", "cmake_minimum_required(VERSION 3.5)\n" + ) + nested_dir = self.nested_parent # contains inner_pkg + + results = _as_str_set( + SUT.find_package_xml_files([self.pkg1 / "package.xml", cmake, nested_dir]) + ) + expected = { + str(self.pkg1 / "package.xml"), + str(self.inner_pkg / "package.xml"), + } + self.assertEqual(results, expected) + + def test_find_package_xml_files_directory_recursion(self): + """Scanning /src should locate all package.xml files recursively (no colcon filtering).""" + results = _as_str_set(SUT.find_package_xml_files([self.ws / "src"])) + + expected = { + str(self.pkg1 / "package.xml"), + str(self.pkg_nameless / "package.xml"), + str(self.inner_pkg / "package.xml"), + str(self.pkg_ignored / "package.xml"), # present despite COLCON_IGNORE + str(self.pkg_third / "package.xml"), # present despite parent COLCON_IGNORE + } + self.assertEqual(results, expected) + + def test_find_package_xml_files_ignores_nonexistent_and_deduplicates(self): + """Nonexistent inputs are ignored; duplicates (file + dir) collapse to one.""" + results = _as_str_set( + SUT.find_package_xml_files( + [self.pkg1 / "package.xml", self.pkg1, self.t.root / "nope/nope"] + ) + ) + self.assertEqual(results, {str(self.pkg1 / "package.xml")}) + # --- CLI main() --------------------------------------------------------- def test_main_lists_packages_names_only(self): argv = ["prog", str(self.pkg1)] From d854e6d55d1d6d01bf2eaad301ffcdb56b8c0885 Mon Sep 17 00:00:00 2001 From: Aljoscha Schmidt Date: Mon, 1 Sep 2025 14:42:25 +0200 Subject: [PATCH 2/5] More tests --- Readme.md | 3 +- .../package_xml_validator.py | 41 +----- .../pkg_missing_test_depends/CMakeLists.txt | 66 +++++++++ .../pkg_missing_test_depends/package.xml | 26 ++++ .../pkg_no_buildtype_tag/package.xml | 3 +- .../pkg_no_buildtype_tag/package.xml | 5 +- .../pkg_correct/package.xml | 1 + .../pkg_missing_launch_deps/package.xml | 1 + tests/test_package_xml_formatted_cli.py | 5 +- tests/test_pkg_missing_launch_deps.py | 129 +++++++++++++----- tests/test_pky_type_dependent_validation.py | 102 ++++++++++---- 11 files changed, 272 insertions(+), 110 deletions(-) create mode 100644 tests/examples/cmake_specific_tests/hector_gamepad_manager/pkg_missing_test_depends/CMakeLists.txt create mode 100644 tests/examples/cmake_specific_tests/hector_gamepad_manager/pkg_missing_test_depends/package.xml diff --git a/Readme.md b/Readme.md index 84b0c90..52b05ae 100644 --- a/Readme.md +++ b/Readme.md @@ -52,7 +52,7 @@ Validates and formats `package.xml` files to enforce consistency and ROS 2 schem ## 🛠️ Usage Example ```bash -package-xml-validator [-h] [--check-only] [--file FILE] [--verbose] [--check-with-xmllint] [--skip-rosdep-key-validation] [--compare-with-cmake] [src ...] +package-xml-validator [-h] [--check-only] [--file FILE] [--verbose] [--skip-rosdep-key-validation] [--compare-with-cmake] [src ...] Validate and format ROS2 package.xml files. @@ -64,7 +64,6 @@ options: --check-only Only check for errors without correcting. --file FILE Path to a single XML file to process. If provided, 'src' arguments are ignored. --verbose Enable verbose output. - --check-with-xmllint Recheck XML schema using xmllint. --skip-rosdep-key-validation Check if rosdeps are valid. --compare-with-cmake Check if all CMake dependencies are in package.xml. --auto-fill-missing-deps Automatically fill missing dependencies in package.xml. diff --git a/package_xml_validation/package_xml_validator.py b/package_xml_validation/package_xml_validator.py index dd756be..716bd59 100644 --- a/package_xml_validation/package_xml_validator.py +++ b/package_xml_validation/package_xml_validator.py @@ -17,7 +17,6 @@ from helpers.cmake_parsers import read_deps_from_cmake_file from helpers.find_launch_dependencies import scan_files from helpers.workspace import find_package_xml_files -import subprocess import re @@ -31,7 +30,6 @@ class PackageXmlValidator: def __init__( self, check_only=False, - check_with_xmllint=False, check_rosdeps=True, compare_with_cmake=False, auto_fill_missing_deps=False, @@ -40,7 +38,6 @@ def __init__( ): self.verbose = verbose self.check_only = check_only - self.check_with_xmllint = check_with_xmllint self.check_rosdeps = check_rosdeps self.compare_with_cmake = compare_with_cmake if self.compare_with_cmake and not self.check_rosdeps: @@ -54,7 +51,7 @@ def __init__( self.rosdep_validator = RosdepValidator(pkg_path=path) self.formatter = PackageXmlFormatter( check_only=check_only, - check_with_xmllint=check_with_xmllint, + check_with_xmllint=False, verbose=verbose, ) self.encountered_unresolvable_error = False @@ -64,8 +61,6 @@ def __init__( self.check_count = 1 if self.check_rosdeps: self.num_checks += 1 - if self.check_with_xmllint: - self.num_checks += 1 if self.compare_with_cmake: self.num_checks += 1 @@ -120,7 +115,7 @@ def check_for_cmake( pkg_name = os.path.basename(os.path.dirname(xml_file)) valid_xml = True build_deps_cmake, test_deps_cmake = read_deps_from_cmake_file(cmake_file) - # make sure that all cmake dependencies are in the package.xml if they can be resolved + # ---------------------------- BUILD DEPENDENCIES ---------------------------- unresolvable = self.rosdep_validator.check_rosdeps_and_local_pkgs( build_deps_cmake ) @@ -145,7 +140,7 @@ def check_for_cmake( unresolvable = self.rosdep_validator.check_rosdeps_and_local_pkgs( test_deps_cmake ) - + # ---------------------------- TEST DEPENDENCIES ---------------------------- missing_deps = [ dep for dep in test_deps_cmake @@ -165,24 +160,6 @@ def check_for_cmake( self.formatter.add_dependencies(root, missing_deps, "test_depend") return valid_xml - def validate_xml_with_xmllint(self, xml_file): - """Validate XML file against the ROS package_format3.xsd schema using xmllint.""" - schema_url = "http://download.ros.org/schema/package_format3.xsd" - try: - result = subprocess.run( - ["xmllint", "--noout", "--schema", schema_url, xml_file], - capture_output=True, - text=True, - ) - if result.returncode != 0: - self.logger.error(f"XML validation error in {xml_file}:") - self.logger.error(result.stderr) - return False - return True - except Exception as e: - self.logger.error(f"Error running xmllint on {xml_file}: {e}") - return False - def validate_launch_dependencies( self, root, @@ -530,12 +507,6 @@ def check_and_format_files(self, package_xml_files): "Check ROS dependencies", self.check_for_rosdeps, rosdeps, xml_file ) self.encountered_unresolvable_error |= not valid - # Check with xmllint if enabled - if self.check_with_xmllint: - valid = self.perform_check( - "Check with xmllint", self.validate_xml_with_xmllint, xml_file - ) - self.encountered_unresolvable_error |= not valid # Check for CMake dependencies if enabled if self.compare_with_cmake: build_deps = self.formatter.retrieve_build_dependencies(root) @@ -607,11 +578,6 @@ def main(): parser.add_argument("--verbose", action="store_true", help="Enable verbose output.") - parser.add_argument( - "--check-with-xmllint", - action="store_true", - help="Recheck XML schema using xmllint.", - ) parser.add_argument( "--skip-rosdep-key-validation", action="store_true", @@ -651,7 +617,6 @@ def main(): formatter = PackageXmlValidator( check_only=args.check_only, verbose=args.verbose, - check_with_xmllint=args.check_with_xmllint, check_rosdeps=not args.skip_rosdep_key_validation, compare_with_cmake=args.compare_with_cmake, auto_fill_missing_deps=args.auto_fill_missing_deps, diff --git a/tests/examples/cmake_specific_tests/hector_gamepad_manager/pkg_missing_test_depends/CMakeLists.txt b/tests/examples/cmake_specific_tests/hector_gamepad_manager/pkg_missing_test_depends/CMakeLists.txt new file mode 100644 index 0000000..5b87c87 --- /dev/null +++ b/tests/examples/cmake_specific_tests/hector_gamepad_manager/pkg_missing_test_depends/CMakeLists.txt @@ -0,0 +1,66 @@ +cmake_minimum_required(VERSION 3.8) +project(hector_gamepad_manager) + +if (NOT CMAKE_CXX_STANDARD) + set(CMAKE_CXX_STANDARD 17) + set(CMAKE_CXX_STANDARD_REQUIRED ON) +endif() + +if (CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang") + add_compile_options(-Wall -Wextra -Wpedantic) +endif() + +find_package(ament_cmake REQUIRED) +set(DEPENDENCIES + pluginlib + rclcpp + sensor_msgs + yaml-cpp + hector_gamepad_plugin_interface +) + +foreach(Dependency IN LISTS DEPENDENCIES) + find_package(${Dependency} REQUIRED) +endforeach() + +set(HEADERS + include/hector_gamepad_manager/hector_gamepad_manager.hpp +) + +set(SOURCES + src/hector_gamepad_manager.cpp +) + +add_library(${PROJECT_NAME} SHARED ${SOURCES} ${HEADERS}) +target_include_directories(${PROJECT_NAME} PUBLIC + $ + $ +) +target_link_libraries(${PROJECT_NAME} PUBLIC yaml-cpp) +ament_target_dependencies(${PROJECT_NAME} PUBLIC ${DEPENDENCIES}) + +add_executable(hector_gamepad_manager_node src/hector_gamepad_manager_node.cpp) +target_link_libraries(hector_gamepad_manager_node PUBLIC ${PROJECT_NAME}) + +install(TARGETS ${PROJECT_NAME} EXPORT ${PROJECT_NAME}-targets LIBRARY DESTINATION lib) +install(DIRECTORY include/ DESTINATION include) +install(TARGETS hector_gamepad_manager_node DESTINATION lib/${PROJECT_NAME}) +install(DIRECTORY launch config DESTINATION share/${PROJECT_NAME}) + + +if(BUILD_TESTING) + find_package(ament_cmake_gtest REQUIRED) + find_package(ros_testing REQUIRED) + ament_add_gtest_executable(${PROJECT_NAME}_test + test/test_hector_gamepad_manager.cpp + ) + target_link_libraries(${PROJECT_NAME}_test ${PROJECT_NAME}) + ament_target_dependencies(${PROJECT_NAME}_test ${THIS_PACKAGE_INCLUDE_DEPENDS}) + add_ros_test(test/test_hector_gamepad_manager.launch.py ARGS "test_binary_dir:=${CMAKE_CURRENT_BINARY_DIR}") +endif () + + +ament_export_targets(${PROJECT_NAME}-targets) +ament_export_include_directories(include) +ament_export_dependencies(${DEPENDENCIES}) +ament_package() diff --git a/tests/examples/cmake_specific_tests/hector_gamepad_manager/pkg_missing_test_depends/package.xml b/tests/examples/cmake_specific_tests/hector_gamepad_manager/pkg_missing_test_depends/package.xml new file mode 100644 index 0000000..6ce28bf --- /dev/null +++ b/tests/examples/cmake_specific_tests/hector_gamepad_manager/pkg_missing_test_depends/package.xml @@ -0,0 +1,26 @@ + + + + hector_gamepad_manager + 0.0.0 + Package for managing gamepad inputs + Simon Giegerich + TODO: License declaration + + ament_cmake + + hector_gamepad_plugin_interface + pluginlib + rclcpp + sensor_msgs + yaml-cpp + + hector_gamepad_manager_plugins + + ament_lint_auto + ament_lint_common + + + ament_cmake + + diff --git a/tests/examples/export_tag_examples/ament_cmake/pkg_no_buildtype_tag/package.xml b/tests/examples/export_tag_examples/ament_cmake/pkg_no_buildtype_tag/package.xml index 2c8d14d..bcb630f 100644 --- a/tests/examples/export_tag_examples/ament_cmake/pkg_no_buildtype_tag/package.xml +++ b/tests/examples/export_tag_examples/ament_cmake/pkg_no_buildtype_tag/package.xml @@ -22,6 +22,5 @@ ament_lint_common ros_testing - - + diff --git a/tests/examples/export_tag_examples/ament_python/pkg_no_buildtype_tag/package.xml b/tests/examples/export_tag_examples/ament_python/pkg_no_buildtype_tag/package.xml index cfab524..330be16 100644 --- a/tests/examples/export_tag_examples/ament_python/pkg_no_buildtype_tag/package.xml +++ b/tests/examples/export_tag_examples/ament_python/pkg_no_buildtype_tag/package.xml @@ -1,4 +1,4 @@ - + python_pkg @@ -12,7 +12,6 @@ rclpy std_msgs - - + diff --git a/tests/examples/launch_pkg_examples/dynamixel_ros_control/pkg_correct/package.xml b/tests/examples/launch_pkg_examples/dynamixel_ros_control/pkg_correct/package.xml index 958de5c..a881020 100644 --- a/tests/examples/launch_pkg_examples/dynamixel_ros_control/pkg_correct/package.xml +++ b/tests/examples/launch_pkg_examples/dynamixel_ros_control/pkg_correct/package.xml @@ -23,6 +23,7 @@ std_msgs std_srvs transmission_interface + yaml-cpp yaml_cpp_vendor controller_manager diff --git a/tests/examples/launch_pkg_examples/dynamixel_ros_control/pkg_missing_launch_deps/package.xml b/tests/examples/launch_pkg_examples/dynamixel_ros_control/pkg_missing_launch_deps/package.xml index af85b85..40da574 100644 --- a/tests/examples/launch_pkg_examples/dynamixel_ros_control/pkg_missing_launch_deps/package.xml +++ b/tests/examples/launch_pkg_examples/dynamixel_ros_control/pkg_missing_launch_deps/package.xml @@ -23,6 +23,7 @@ std_msgs std_srvs transmission_interface + yaml-cpp yaml_cpp_vendor ament_lint_auto diff --git a/tests/test_package_xml_formatted_cli.py b/tests/test_package_xml_formatted_cli.py index 94ae284..1a4f5e7 100644 --- a/tests/test_package_xml_formatted_cli.py +++ b/tests/test_package_xml_formatted_cli.py @@ -134,9 +134,7 @@ def test_cli_verbose_and_xmllint_flags_propagate(self): """--verbose and --check-with-xmllint should propagate to validator args.""" fake_cls, constructed = make_fake_validator_factory() with mock.patch.object(SUT, "PackageXmlValidator", fake_cls): - with mock.patch.object( - sys, "argv", ["prog", "--verbose", "--check-with-xmllint"] - ): + with mock.patch.object(sys, "argv", ["prog", "--verbose"]): with mock.patch.dict(os.environ, {"ROS_DISTRO": "jazzy"}, clear=True): buf = io.StringIO() with redirect_stdout(buf): @@ -145,7 +143,6 @@ def test_cli_verbose_and_xmllint_flags_propagate(self): self.assertEqual(len(constructed), 1) inst = constructed[0] self.assertTrue(inst.init_kwargs["verbose"]) - self.assertTrue(inst.init_kwargs["check_with_xmllint"]) self.assertTrue(inst.init_kwargs["check_rosdeps"]) def test_cli_exit_code_on_failure(self): diff --git a/tests/test_pkg_missing_launch_deps.py b/tests/test_pkg_missing_launch_deps.py index ae935cf..600b1b8 100644 --- a/tests/test_pkg_missing_launch_deps.py +++ b/tests/test_pkg_missing_launch_deps.py @@ -1,5 +1,6 @@ import os import unittest +from unittest.mock import MagicMock import tempfile import shutil import subprocess @@ -7,6 +8,13 @@ from package_xml_validation.package_xml_validator import ( PackageXmlValidator, ) +from enum import Enum + + +class FormatterType(Enum): + CHECK_ONLY = "check_only" + NO_AUTO_FILL = "no_auto_fill" + FULL = "full" def validate_xml_with_xmllint(xml_file): @@ -38,13 +46,35 @@ def setUpClass(cls): """ current_dir = os.path.dirname(__file__) cls.examples_dir = os.path.join(current_dir, "examples", "launch_pkg_examples") - cls.formatter = PackageXmlValidator( + cls.formatters = {} + cls.formatters[FormatterType.FULL] = PackageXmlValidator( check_only=False, verbose=True, auto_fill_missing_deps=True, - check_rosdeps=False, + check_rosdeps=True, + compare_with_cmake=True, + ) + cls.formatters[FormatterType.CHECK_ONLY] = PackageXmlValidator( + check_only=True, + verbose=True, + auto_fill_missing_deps=False, + check_rosdeps=True, + compare_with_cmake=False, ) + cls.formatters[FormatterType.NO_AUTO_FILL] = PackageXmlValidator( + check_only=False, + verbose=True, + auto_fill_missing_deps=False, + check_rosdeps=True, + compare_with_cmake=False, + ) + for formatter in cls.formatters.values(): + formatter.rosdep_validator = MagicMock() + formatter.rosdep_validator.check_rosdeps_and_local_pkgs = MagicMock( + return_value=[] + ) + def setUp(self): """ Create a temporary directory for each test, copy example files into it, @@ -95,6 +125,16 @@ def _compare_xml_files(self, file1: str, file2: str) -> bool: print(f"XML Syntax Error: {e}") return False + def _compare_xml_files_and_print(self, file1: str, file2: str) -> bool: + result = self._compare_xml_files(file1, file2) + if not result: + print(f"XML files do not match: {file1} != {file2}") + with open(file1, encoding="utf-8") as f1: + print(f"Contents of {file1}:\n{f1.read()}") + with open(file2, encoding="utf-8") as f2: + print(f"Contents of {file2}:\n{f2.read()}") + return result + def test_xml_formatting(self): """ Iterate over all example packages in the test directory, @@ -104,45 +144,60 @@ def test_xml_formatting(self): correct_xml = os.path.join( self.examples_dir, example_pkg, "pkg_correct", "package.xml" ) - build_type_dir = os.path.join(self.test_dir, example_pkg) - for pkg in os.listdir(build_type_dir): - xml_file = os.path.join(build_type_dir, pkg, "package.xml") - - # Use subTest to continue testing other files even if this one fails - with self.subTest(example_pkg=example_pkg, pkg=pkg, xml_file=xml_file): - # apply the formatter - with open(xml_file) as f: - xml_content = f.read() - valid = self.formatter.check_and_format_files([xml_file]) - msg = "" - - if not pkg == "pkg_correct": - if valid: - with open(xml_file) as f: - msg = f"Formatted XML file {xml_file}:\n'{f.read()}'" - self.assertFalse( - valid, - f"XML file {xml_file} is expected to be invalid but was valid. {msg} \n vs original: \n{xml_content}", + pkg_dir = os.path.join(self.test_dir, example_pkg) + for pkg in os.listdir(pkg_dir): + xml_file = os.path.join(pkg_dir, pkg, "package.xml") + for type, formatter in self.formatters.items(): + # Use subTest to continue testing other files even if this one fails + with self.subTest(example_pkg=example_pkg, pkg=pkg, type=type): + # recopy file from example to test dir + shutil.copy2( + os.path.join( + self.examples_dir, example_pkg, pkg, "package.xml" + ), + xml_file, + ) + # apply the formatter + with open(xml_file) as f: + xml_content = f.read() + valid = formatter.check_and_format_files([xml_file]) + msg = "" + + if not pkg == "pkg_correct": + if valid: + with open(xml_file) as f: + msg = ( + f"Formatted XML file {xml_file}:\n'{f.read()}'" + ) + self.assertFalse( + valid, + f"XML file {xml_file} is expected to be invalid but was valid. {msg} \n vs original: \n{xml_content}", + ) + else: + if not valid: + with open(xml_file) as f: + msg = f"Invalid XML file {xml_file}:\n{f.read()}" + self.assertTrue( + valid, + f"XML file {xml_file} is expected to be valid but was invalid. {msg} \n vs original: \n{xml_content}", + ) + original_xml = os.path.join( + self.examples_dir, example_pkg, pkg, "package.xml" + ) + # only when using the FULL formatter the missing parts should be added + expected_xml = ( + correct_xml if type == FormatterType.FULL else original_xml ) - else: - if not valid: - with open(xml_file) as f: - msg = f"Invalid XML file {xml_file}:\n{f.read()}" self.assertTrue( - valid, - f"XML file {xml_file} is expected to be valid but was invalid. {msg} \n vs original: \n{xml_content}", + self._compare_xml_files_and_print(xml_file, expected_xml), + f"XML files do not match: {xml_file} != {expected_xml} (using formatter type: {type})", ) - self.assertTrue( - self._compare_xml_files(xml_file, correct_xml), - f"XML files do not match: {xml_file} != {correct_xml}", - ) - - # validate the XML file with xmllint - self.assertTrue( - validate_xml_with_xmllint(xml_file), - f"XML file {xml_file} failed xmllint validation.", - ) + # validate the XML file with xmllint + self.assertTrue( + validate_xml_with_xmllint(xml_file), + f"XML file {xml_file} failed xmllint validation.", + ) if __name__ == "__main__": diff --git a/tests/test_pky_type_dependent_validation.py b/tests/test_pky_type_dependent_validation.py index 7264d30..6d5490a 100644 --- a/tests/test_pky_type_dependent_validation.py +++ b/tests/test_pky_type_dependent_validation.py @@ -9,6 +9,14 @@ from package_xml_validation.package_xml_validator import ( PackageXmlValidator, ) +from enum import Enum + + +# enum class Formatter Type +class FormatterType(Enum): + CHECK_ONLY = "check_only" + NO_AUTO_FILL = "no_auto_fill" + FULL = "full" def validate_xml_with_xmllint(xml_file): @@ -40,18 +48,34 @@ def setUpClass(cls): """ current_dir = os.path.dirname(__file__) cls.examples_dir = os.path.join(current_dir, "examples", "export_tag_examples") - cls.formatter = PackageXmlValidator( + cls.formatters = {} + cls.formatters[FormatterType.FULL] = PackageXmlValidator( check_only=False, verbose=True, auto_fill_missing_deps=True, check_rosdeps=True, compare_with_cmake=True, ) + cls.formatters[FormatterType.CHECK_ONLY] = PackageXmlValidator( + check_only=True, + verbose=True, + auto_fill_missing_deps=False, + check_rosdeps=True, + compare_with_cmake=False, + ) - cls.formatter.rosdep_validator = MagicMock() - cls.formatter.rosdep_validator.check_rosdeps_and_local_pkgs = MagicMock( - return_value=[] + cls.formatters[FormatterType.NO_AUTO_FILL] = PackageXmlValidator( + check_only=False, + verbose=True, + auto_fill_missing_deps=False, + check_rosdeps=True, + compare_with_cmake=False, ) + for formatter in cls.formatters.values(): + formatter.rosdep_validator = MagicMock() + formatter.rosdep_validator.check_rosdeps_and_local_pkgs = MagicMock( + return_value=[] + ) def setUp(self): """ @@ -99,6 +123,16 @@ def _compare_xml_files(self, file1: str, file2: str) -> bool: print(f"XML Syntax Error: {e}") return False + def _compare_xml_files_and_print(self, file1: str, file2: str) -> bool: + result = self._compare_xml_files(file1, file2) + if not result: + print(f"XML files do not match: {file1} != {file2}") + with open(file1, encoding="utf-8") as f1: + print(f"Contents of {file1}:\n{f1.read()}") + with open(file2, encoding="utf-8") as f2: + print(f"Contents of {file2}:\n{f2.read()}") + return result + def test_xml_formatting(self): """ Iterate over all example packages in the test directory, @@ -112,26 +146,46 @@ def test_xml_formatting(self): for pkg in os.listdir(build_type_dir): xml_file = os.path.join(build_type_dir, pkg, "package.xml") # apply the formatter - valid = self.formatter.check_and_format_files([xml_file]) - if not pkg == "pkg_correct": - self.assertFalse( - valid, - f"XML file {xml_file} is expected to be invalid but was valid.", - ) - else: - self.assertTrue( - valid, - f"XML file {xml_file} is expected to be valid but was invalid.", - ) - self.assertTrue( - self._compare_xml_files(xml_file, correct_xml), - f"XML files do not match: {xml_file} != {correct_xml}", - ) - # validate the XML file with xmllint - self.assertTrue( - validate_xml_with_xmllint(xml_file), - f"XML file {xml_file} failed xmllint validation.", - ) + for type, formatter in self.formatters.items(): + with self.subTest( + pkg=pkg, + build_type=build_type, + formatter_type=type.value, + ): + # re-copy file from examples_dir to test_dir + shutil.copy2( + os.path.join( + self.examples_dir, build_type, pkg, "package.xml" + ), + xml_file, + ) + valid = formatter.check_and_format_files([xml_file]) + if not pkg == "pkg_correct": + self.assertFalse( + valid, + f"XML file {xml_file} is expected to be invalid but was valid. (using formatter: {type.value})", + ) + else: + self.assertTrue( + valid, + f"XML file {xml_file} is expected to be valid but was invalid. (using formatter: {type.value})", + ) + original_xml = os.path.join( + self.examples_dir, build_type, pkg, "package.xml" + ) + # only when using the FULL formatter the missing parts should be added + expected_xml = ( + correct_xml if type == FormatterType.FULL else original_xml + ) + self.assertTrue( + self._compare_xml_files_and_print(xml_file, expected_xml), + f"XML files do not match: {xml_file} != {expected_xml}", + ) + # validate the XML file with xmllint + self.assertTrue( + validate_xml_with_xmllint(xml_file), + f"XML file {xml_file} failed xmllint validation.", + ) if __name__ == "__main__": From 12d87ae7ab0cbd09e5e165a777eaf01a85d28464 Mon Sep 17 00:00:00 2001 From: Aljoscha Schmidt Date: Mon, 1 Sep 2025 15:02:23 +0200 Subject: [PATCH 3/5] Test that no launch deps added when rosdep not working --- tests/test_pkg_missing_launch_deps.py | 37 ++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/tests/test_pkg_missing_launch_deps.py b/tests/test_pkg_missing_launch_deps.py index 600b1b8..b197153 100644 --- a/tests/test_pkg_missing_launch_deps.py +++ b/tests/test_pkg_missing_launch_deps.py @@ -15,6 +15,7 @@ class FormatterType(Enum): CHECK_ONLY = "check_only" NO_AUTO_FILL = "no_auto_fill" FULL = "full" + ALL_ROSDEPS_ARE_UNRESOLVABLE = "all_rosdeps_are_unresolvable" def validate_xml_with_xmllint(xml_file): @@ -69,11 +70,27 @@ def setUpClass(cls): check_rosdeps=True, compare_with_cmake=False, ) - for formatter in cls.formatters.values(): - formatter.rosdep_validator = MagicMock() - formatter.rosdep_validator.check_rosdeps_and_local_pkgs = MagicMock( - return_value=[] + cls.formatters[FormatterType.ALL_ROSDEPS_ARE_UNRESOLVABLE] = ( + PackageXmlValidator( + check_only=False, + verbose=True, + auto_fill_missing_deps=True, + check_rosdeps=True, + compare_with_cmake=True, ) + ) + for key, formatter in cls.formatters.items(): + formatter.rosdep_validator = MagicMock() + if key is FormatterType.ALL_ROSDEPS_ARE_UNRESOLVABLE: + formatter.rosdep_validator.check_rosdeps_and_local_pkgs = MagicMock( + side_effect=lambda deps: deps + ) + # Mock "normal" rosdep check, we are testing launch deps here + formatter.check_for_rosdeps = MagicMock(return_value=True) + else: + formatter.rosdep_validator.check_rosdeps_and_local_pkgs = MagicMock( + side_effect=lambda deps: [] # everything resolvable + ) def setUp(self): """ @@ -130,9 +147,15 @@ def _compare_xml_files_and_print(self, file1: str, file2: str) -> bool: if not result: print(f"XML files do not match: {file1} != {file2}") with open(file1, encoding="utf-8") as f1: - print(f"Contents of {file1}:\n{f1.read()}") + content_1 = f1.readlines() with open(file2, encoding="utf-8") as f2: - print(f"Contents of {file2}:\n{f2.read()}") + content_2 = f2.readlines() + for line1, line2 in zip(content_1, content_2): + are_equal = line1 == line2 + if are_equal: + print(f" {line1.rstrip()}") + else: + print(f"-> {line1.rstrip()} != {line2.rstrip()}") return result def test_xml_formatting(self): @@ -173,6 +196,7 @@ def test_xml_formatting(self): valid, f"XML file {xml_file} is expected to be invalid but was valid. {msg} \n vs original: \n{xml_content}", ) + else: if not valid: with open(xml_file) as f: @@ -181,6 +205,7 @@ def test_xml_formatting(self): valid, f"XML file {xml_file} is expected to be valid but was invalid. {msg} \n vs original: \n{xml_content}", ) + self._compare_xml_files_and_print(xml_file, correct_xml) original_xml = os.path.join( self.examples_dir, example_pkg, pkg, "package.xml" ) From c1fa27992dfef6e9eed0a7aab27c04db381aedeb Mon Sep 17 00:00:00 2001 From: Aljoscha Schmidt Date: Mon, 1 Sep 2025 15:04:33 +0200 Subject: [PATCH 4/5] Added comment --- tests/test_pkg_missing_launch_deps.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/test_pkg_missing_launch_deps.py b/tests/test_pkg_missing_launch_deps.py index b197153..4859151 100644 --- a/tests/test_pkg_missing_launch_deps.py +++ b/tests/test_pkg_missing_launch_deps.py @@ -18,6 +18,14 @@ class FormatterType(Enum): ALL_ROSDEPS_ARE_UNRESOLVABLE = "all_rosdeps_are_unresolvable" +# GOAl +# Check that launch dependencies are correctly added to the package.xml +# CHECK-ONLY -> find missing but DO NOT CHANGE THE FILE +# NO-AUTO-FILL -> find missing and do not autofill +# FULL -> find missing and autofill +# ALL-ROSDEPS-ARE-UNRESOLVABLE -> find missing but cannot autofill due to unresolvable rosdeps + + def validate_xml_with_xmllint(xml_file): """Validate XML file against the ROS package_format3.xsd schema using xmllint.""" schema_url = "http://download.ros.org/schema/package_format3.xsd" From 4a551495b7a949054e357cec80e18947518376d9 Mon Sep 17 00:00:00 2001 From: Aljoscha Schmidt Date: Mon, 1 Sep 2025 15:12:21 +0200 Subject: [PATCH 5/5] More Tests --- .../helpers/find_launch_dependencies.py | 7 ++----- tests/test_find_launch_dependencies.py | 18 +++++++++++++++++- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/package_xml_validation/helpers/find_launch_dependencies.py b/package_xml_validation/helpers/find_launch_dependencies.py index 5a3a514..6588cd2 100644 --- a/package_xml_validation/helpers/find_launch_dependencies.py +++ b/package_xml_validation/helpers/find_launch_dependencies.py @@ -35,11 +35,8 @@ def scan_file(path, found: set[str], verbose: bool = False): """Apply every regex to the file and add matches to `found`.""" - try: - with open(path, encoding="utf-8") as f: - text = f.read() - except (UnicodeDecodeError, OSError): - return + with open(path, encoding="utf-8") as f: + text = f.read() for i, rx in enumerate(COMPILED): for m in rx.finditer(text): diff --git a/tests/test_find_launch_dependencies.py b/tests/test_find_launch_dependencies.py index de14364..3992852 100644 --- a/tests/test_find_launch_dependencies.py +++ b/tests/test_find_launch_dependencies.py @@ -1,7 +1,10 @@ import os import unittest -from package_xml_validation.helpers.find_launch_dependencies import scan_file +from package_xml_validation.helpers.find_launch_dependencies import ( + scan_file, + scan_files, +) class TestFindLaunchDependencies(unittest.TestCase): @@ -50,6 +53,19 @@ def test_scan_each_file(self): ), ) + def test_scan_each_file_given_file(self): + """When given a file path, or an not existing directory scan_files should return an empty list""" + found = scan_files("non_existing_file.launch.py") + self.assertEqual(len(found), 0) + existing_file = os.path.join( + os.path.dirname(__file__), + "examples", + "launch_examples", + "python_example.launch.py", + ) + found = scan_files(existing_file) + self.assertEqual(len(found), 0) + if __name__ == "__main__": unittest.main()