Check for Invisible Changes (idempotency) (MetroRobots#12)

* Add idempotency tests * Remove invisible changes
edu-rossrobotics · Jan 12, 2024 · 7e35fcd · 7e35fcd
1 parent 4a1d4c1
commit 7e35fcd
Show file tree

Hide file tree

Showing 6 changed files with 57 additions and 33 deletions.
diff --git a/src/ros_glint/glinters/cmake.py b/src/ros_glint/glinters/cmake.py
@@ -445,7 +445,6 @@ def remove_old_style_cpp_dependencies(package):
     def remove_pattern(section, pattern):
         prev_len = len(section.values)
         section.values = [v for v in section.values if pattern not in v]
-        section.mark_changed()
         return prev_len != len(section.values)
 
     if not package.cmake:

diff --git a/src/ros_glint/glinters/cmake_pretty.py b/src/ros_glint/glinters/cmake_pretty.py
@@ -6,6 +6,14 @@
 SHOULD_ALPHABETIZE = ['COMPONENTS', 'DEPENDENCIES', 'FILES', 'CATKIN_DEPENDS']
 
 
+def set_style_attribute(section, attribute, new_value):
+    existing_value = getattr(section.style, attribute)
+    if existing_value == new_value:
+        return
+    setattr(section.style, attribute, new_value)
+    section.mark_changed()
+
+
 @glinter
 def alphabetize_cmake_sections(package):
     if not package.cmake:
@@ -32,8 +40,7 @@ def prettify_catkin_package_cmd(package):
         return
     for cmd in package.cmake.content_map['catkin_package']:
         for section in cmd.get_real_sections():
-            section.style.prename = NEWLINE_PLUS_4
-        cmd.mark_changed()
+            set_style_attribute(section, 'prename', NEWLINE_PLUS_4)
 
 
 @glinter
@@ -54,7 +61,7 @@ def prettify_package_lists(package):
                     if key not in acceptable_styles:
                         section.style.name_val_sep = NEWLINE_PLUS_8
                         section.style.val_sep = NEWLINE_PLUS_8
-                        cmd.mark_changed()
+                        section.mark_changed()
 
 
 @glinter
@@ -65,26 +72,17 @@ def prettify_msgs_srvs(package):
     for cmd in package.cmake.content_map['add_message_files'] + package.cmake.content_map['add_service_files']:
         for section in cmd.get_real_sections():
             if len(section.values) > 1:
-                section.style.name_val_sep = NEWLINE_PLUS_4
-                section.style.val_sep = NEWLINE_PLUS_4
-            cmd.mark_changed()
+                set_style_attribute(section, 'name_val_sep', NEWLINE_PLUS_4)
+                set_style_attribute(section, 'val_sep', NEWLINE_PLUS_4)
     # ROS 2 version
     for cmd in package.cmake.content_map['rosidl_generate_interfaces']:
         for section in cmd.get_real_sections():
             if section.name == '' and section == cmd.sections[0]:
-                if section.style.val_sep != NEWLINE_PLUS_4:
-                    section.style.val_sep = NEWLINE_PLUS_4
-                    cmd.mark_changed()
+                set_style_attribute(section, 'val_sep', NEWLINE_PLUS_4)
             elif section.name == 'DEPENDENCIES':
-                if section.style.prename != NEWLINE_PLUS_2:
-                    section.style.prename = NEWLINE_PLUS_2
-                    cmd.mark_changed()
-                if section.style.val_sep != NEWLINE_PLUS_4:
-                    section.style.val_sep = NEWLINE_PLUS_4
-                    cmd.mark_changed()
-                if section.style.name_val_sep != NEWLINE_PLUS_4:
-                    section.style.name_val_sep = NEWLINE_PLUS_4
-                    cmd.mark_changed()
+                set_style_attribute(section, 'prename', NEWLINE_PLUS_2)
+                set_style_attribute(section, 'val_sep', NEWLINE_PLUS_4)
+                set_style_attribute(section, 'name_val_sep', NEWLINE_PLUS_4)
 
 
 @glinter
@@ -95,7 +93,7 @@ def prettify_installs(package):
         cmd.sections = [s for s in cmd.sections if not isinstance(s, str)]
         if not cmd.sections:
             continue
-        cmd.mark_changed()
+
         if '\n' in cmd.sections[0].style.prename:
             new_style = cmd.sections[0].style.prename
         else:
@@ -104,18 +102,16 @@ def prettify_installs(package):
         zeroed = False
         for section in cmd.sections[1:]:
             if len(section.values) == 0:
-                section.style.prename = new_style
+                set_style_attribute(section, 'prename', new_style)
                 zeroed = True
             elif not zeroed:
-                section.style.prename = new_style
+                set_style_attribute(section, 'prename', new_style)
             else:
-                section.style.prename = ''
+                set_style_attribute(section, 'prename', '')
 
     for cmd in package.cmake.content_map['catkin_install_python']:
         section = cmd.sections[1]
-        if section.style.prename != CATKIN_INSTALL_PYTHON_PRENAME:
-            section.style.prename = CATKIN_INSTALL_PYTHON_PRENAME
-            cmd.mark_changed()
+        set_style_attribute(section, 'prename', CATKIN_INSTALL_PYTHON_PRENAME)
 
 
 @glinter

diff --git a/src/ros_glint/glinters/cmake_sorting.py b/src/ros_glint/glinters/cmake_sorting.py
@@ -8,21 +8,34 @@ def get_clusters(cmake, desired_style):
 
     The clusters are sorted according to the desired style.
     The strings are grouped at the beginning to maintain the newlines and indenting before each Command.
+
+    Can also return None if everything is already sorted
     """
     anchors = get_ordered_build_targets(cmake)
     ordering = get_ordering(desired_style)
     clusters = []
     current = []
+    prev_key = None
+    needs_sorting = False
+
     for content in cmake.contents:
         current.append(content)
         if isinstance(content, str):
             continue
         key = get_sort_key(content, anchors, ordering)
+        if prev_key and key < prev_key:
+            needs_sorting = True
+        prev_key = key
         clusters.append((key, current))
         current = []
     if len(current) > 0:
-        clusters.append((get_sort_key(None, anchors, ordering), current))
+        last_key = get_sort_key(None, anchors, ordering)
+        if prev_key and last_key < prev_key:
+            needs_sorting = True
+        clusters.append((last_key, current))
 
+    if not needs_sorting:
+        return
     return [kv[1] for kv in sorted(clusters, key=lambda kv: kv[0])]
 
 
@@ -35,14 +48,18 @@ def enforce_ordering(cmake, default_style=None):
     else:
         desired_style = CMakeOrderStyle.TEST_FIRST
 
+    # Recurse through CommandGroups
+    for group in cmake.content_map['group']:
+        enforce_ordering(group.contents, default_style)
+
     clusters = get_clusters(cmake, desired_style)
+    if not clusters:  # Already sorted
+        return
 
     cmake.contents = []
     for contents in clusters:
         cmake.contents += contents
 
-    for group in cmake.content_map['group']:
-        enforce_ordering(group.contents, default_style)
     cmake.mark_changed()
 
 

diff --git a/src/ros_glint/glinters/misc.py b/src/ros_glint/glinters/misc.py
@@ -40,6 +40,6 @@ def remove_useless_files(package):
 
 @glinter
 def misc_xml_formatting(package):
-    package.package_xml.changed = True
+    package.package_xml.force_regeneration()
     for config in package.plugin_xml:
-        config.changed = True
+        config.force_regeneration()
diff --git a/src/ros_glint/glinters/ros_interfaces.py b/src/ros_glint/glinters/ros_interfaces.py
@@ -10,7 +10,7 @@
 def clean_whitespace_from_interface_definition(package):
     # Formerly remove_trailing_whitespace_from_generators
     for interface in package.get_ros_interfaces():
-        interface.changed = True
+        interface.force_regeneration()
 
 
 @glinter

diff --git a/test/test_from_zip.py b/test/test_from_zip.py
@@ -49,6 +49,8 @@ def run_case(test_config, cases):
                 fne(pkg_obj, config=local_config)
             else:
                 fne(pkg_obj)
+
+        components_with_changes = [comp.rel_fn for comp in pkg_obj if comp.changed]
         pkg_obj.save()
 
         s = '{:25} >> {:25} {}'.format(test_config['in'], test_config['out'],
@@ -70,7 +72,10 @@ def run_case(test_config, cases):
             contents_in = pkg_in.get_contents(filename).rstrip()
             contents_out = pkg_out.get_contents(filename).rstrip()
 
-            if contents_in != contents_out:
+            if contents_in == contents_out:
+                if filename in components_with_changes and test_config['in'] == test_config['out']:
+                    problems.append(f'File {filename} has invisible changes')
+            else:
                 print(get_diff_string(contents_in, contents_out, filename))
                 problems.append('The contents of {} do not match!'.format(filename))
 
@@ -92,3 +97,10 @@ def run_case(test_config, cases):
 @pytest.mark.parametrize('test_config, test_data', parameters, ids=test_ids)
 def test_from_zip(test_config, test_data):
     run_case(test_config, test_data)
+
+
+@pytest.mark.parametrize('test_config, test_data', parameters, ids=test_ids)
+def test_idempotency_from_zip(test_config, test_data):
+    test_config_x = dict(test_config)
+    test_config_x['in'] = test_config_x['out']
+    run_case(test_config_x, test_data)