Fix/tests (#2)

* convert to ctest and fix negative parsing issue/some precision problems

* fix float formatting test errors

* add missing parameter to docstring

* enable ctest for cmake_build workflow

* enable top-level tests and run tests in ros

* adjust names

.github/workflows/catkin_build.yml

@@ -1,4 +1,4 @@
-name: 'ROS Noetic: Build'
+name: 'ROS Noetic: Build and Test'
 
 on:
   push:
@@ -8,6 +8,7 @@ on:
 
 jobs:
   build:
+    name: Build and Test
     runs-on: ubuntu-latest
     container: ros:noetic-ros-base-focal
     steps:
@@ -48,3 +49,9 @@ jobs:
 
         # Build
         catkin build config_utilities
+
+    - name: Test
+      shell: bash
+      run: |
+          cd $HOME/catkin_ws
+          catkin test config_utilities --test-target test

.github/workflows/cmake_build.yml

@@ -1,6 +1,6 @@
 # This starter workflow is for a CMake project running on a single platform. There is a different starter workflow if you need cross-platform coverage.
 # See: https://github.com/actions/starter-workflows/blob/main/ci/cmake-multi-platform.yml
-name: "CMake: Build"
+name: "CMake: Build and Test"
 
 on:
   push:
@@ -14,6 +14,7 @@ env:
 
 jobs:
   build:
+    name: Build and Test
     # The CMake configure and build commands are platform agnostic and should work equally well on Windows or Mac.
     # You can convert this to a matrix build if you need cross-platform coverage.
     # See: https://docs.github.com/en/free-pro-team@latest/actions/learn-github-actions/managing-complex-workflows#using-a-build-matrix
@@ -46,3 +47,10 @@ jobs:
       # Build your program with the given configuration
       shell: bash
       run: cmake --build ${{github.workspace}}/build
+
+    - name: Test
+      working-directory: ${{github.workspace}}/build
+      shell: bash
+      # Execute tests defined by the CMake configuration.
+      # See https://cmake.org/cmake/help/latest/manual/ctest.1.html for more detail
+      run: ctest -C ${{env.BUILD_TYPE}}

config_utilities/CMakeLists.txt

@@ -68,6 +68,7 @@ if(CONFIG_UTILS_BUILD_DEMOS)
 endif()
 
 if(CONFIG_UTILS_BUILD_TESTS)
+  enable_testing()
   add_subdirectory(test)
 endif()
 

config_utilities/include/config_utilities/internal/string_utils.h

@@ -93,9 +93,10 @@ std::string joinNamespace(const std::string& namespace_1,
  * @brief Formatting of YAML nodes to strings. Most config types can be neatly represented as low-depth yaml nodes, or
  * should otherwise probably be wrapped in a separate confi struct.
  * @param data The data to be formatted.
+ * @param reformat_float Whether to try and print floats with default stream precision
  * @returns The formatted string.
  */
-std::string dataToString(const YAML::Node& data);
+std::string dataToString(const YAML::Node& data, bool reformat_float = false);
 
 /**
  * @brief Find all occurences of a substring in a string.

config_utilities/include/config_utilities/internal/yaml_parser.h

@@ -223,7 +223,9 @@ class YamlParser {
     // NOTE(lschmid): We assume we don't get integers larger than 64 bit. Also bool, uchar, and string are checked
     // separately.
     const int64_t min = node.as<int64_t>();
-    const uint64_t max = node.as<uint64_t>();
+    // note: usigned parsing fails when number is explicitly negative. defaulting to 0 in these cases is the correct
+    // behavior: the number is explicilty signed and negative, and can only overflow via min
+    const uint64_t max = node.as<uint64_t>(0);
     if (min > 0 && max > static_cast<uint64_t>(std::numeric_limits<T>::max())) {
       std::stringstream ss;
       ss << "Value '" << max << "' overflows storage max of '" << std::numeric_limits<T>::max() << "'";

config_utilities/include/config_utilities/settings.h

@@ -72,6 +72,9 @@ struct Settings {
   // If true, store all validated configs for global printing.
   bool store_valid_configs = true;
 
+  // If true, attempts to print floats and float-like fields with default stream precision
+  bool reformat_floats = true;
+
   /* Factory settings */
   // The factory will look for this param to deduce the type of the object to be created.
   std::string factory_type_param_name = "type";

config_utilities/src/asl_formatter.cpp

@@ -180,9 +180,10 @@ std::string AslFormatter::formatField(const FieldInfo& info, size_t indent) cons
   std::string result;
   const size_t print_width = Settings::instance().print_width;
   const size_t global_indent = Settings::instance().print_indent;
+  const auto reformat_floats = Settings::instance().reformat_floats;
 
   // field is the stringified value, The header is the field name.
-  std::string field = dataToString(info.value);
+  std::string field = dataToString(info.value, reformat_floats);
   if (info.is_default && Settings::instance().indicate_default_values) {
     field += " (default)";
   }

config_utilities/src/string_utils.cpp

@@ -36,6 +36,7 @@
 #include "config_utilities/internal/string_utils.h"
 
 #include <algorithm>
+#include <regex>
 #include <sstream>
 
 namespace config::internal {
@@ -88,18 +89,41 @@ std::string joinNamespace(const std::string& namespace_1,
   return joinNamespace(ns_1, delimiter);
 }
 
-std::string dataToString(const YAML::Node& data) {
+std::string scalarToString(const YAML::Node& data, bool reformat_float) {
+  std::stringstream orig;
+  orig << data;
+  if (!reformat_float) {
+    return orig.str();
+  }
+
+  const std::regex float_detector("[+-]?[0-9]*[.][0-9]+");
+  if (!std::regex_search(orig.str(), float_detector)) {
+    return orig.str();  // no reason to reformat if no decimal points
+  }
+
+  double value;
+  try {
+    value = data.as<double>();
+  } catch (const YAML::InvalidNode&) {
+    return orig.str();  // value is some sort of string that can't be parsed as a float
+  }
+
+  // this should have default ostream precision for formatting float
+  std::stringstream ss;
+  ss << value;
+  return ss.str();
+}
+
+std::string dataToString(const YAML::Node& data, bool reformat_float) {
   switch (data.Type()) {
     case YAML::NodeType::Scalar: {
-      // NOTE(lschmid): All YAML scalars should implement the ostream<< operator.
-      std::stringstream ss;
-      ss << data;
-      return ss.str();
+      // scalars require special handling for float precision
+      return scalarToString(data, reformat_float);
     }
     case YAML::NodeType::Sequence: {
       std::string result = "[";
       for (size_t i = 0; i < data.size(); ++i) {
-        result += dataToString(data[i]);
+        result += dataToString(data[i], reformat_float);
         if (i < data.size() - 1) {
           result += ", ";
         }
@@ -112,7 +136,8 @@ std::string dataToString(const YAML::Node& data) {
       bool has_data = false;
       for (const auto& kv_pair : data) {
         has_data = true;
-        result += dataToString(kv_pair.first) + ": " + dataToString(kv_pair.second) + ", ";
+        result +=
+            dataToString(kv_pair.first, reformat_float) + ": " + dataToString(kv_pair.second, reformat_float) + ", ";
       }
       if (has_data) {
         result = result.substr(0, result.length() - 2);

config_utilities/test/CMakeLists.txt

@@ -25,6 +25,7 @@ if(NOT googletest_POPULATED)
       CACHE BOOL "Build SHARED libraries")
 endif()
 
+include(GoogleTest)
 enable_testing()
 
 add_executable(
@@ -46,11 +47,11 @@ add_executable(
   tests/traits.cpp
   tests/validity_checks.cpp
   tests/virtual_config.cpp
-  tests/yaml_parsing.cpp
-)
+  tests/yaml_parsing.cpp)
 target_include_directories(test_${PROJECT_NAME} PRIVATE include)
 target_link_libraries(test_${PROJECT_NAME} PRIVATE ${PROJECT_NAME}
                                                    GTest::gtest_main)
+gtest_discover_tests(test_${PROJECT_NAME} DISCOVERY_MODE PRE_TEST)
 
 if(${CONFIG_UTILS_INSTALL_TESTS})
   # note that ros uses libdir to handle finding executables by package, but this

config_utilities/test/src/default_config.cpp

@@ -116,7 +116,7 @@ YAML::Node DefaultConfig::modifiedValues() {
   YAML::Node data;
   data["i"] = 2;
   data["f"] = -1.f;
-  data["d"] = 3.1415926;
+  data["d"] = 3.14159; // intentionally avoid precision issues
   data["b"] = false;
   data["u8"] = 255;
   data["s"] = "a different test string";
@@ -158,7 +158,7 @@ void DefaultConfig::expectDefaultValues() {
 void DefaultConfig::expectModifiedValues() {
   EXPECT_EQ(i, 2);
   EXPECT_EQ(f, -1.f);
-  EXPECT_EQ(d, 3.1415926);
+  EXPECT_EQ(d, 3.14159);
   EXPECT_EQ(b, false);
   EXPECT_EQ(u8, 255);
   EXPECT_EQ(s, "a different test string");

config_utilities/test/tests/asl_formatter.cpp

@@ -91,22 +91,24 @@ void declare_config(ConfigUsingArrays& config) {
 
 TEST(AslFormatter, DataToString) {
   YAML::Node data = internal::Visitor::getValues(TestConfig()).data;
-  EXPECT_EQ(internal::dataToString(data["i"]), "1");
-  EXPECT_EQ(internal::dataToString(data["f"]), "2.1");
-  EXPECT_EQ(internal::dataToString(data["d"]), "3.2");
-  EXPECT_EQ(internal::dataToString(data["b"]), "true");
-  EXPECT_EQ(internal::dataToString(data["u8"]), "4");
-  EXPECT_EQ(internal::dataToString(data["s"]), "test string");
-  EXPECT_EQ(internal::dataToString(data["vec"]), "[1, 2, 3]");
-  EXPECT_EQ(internal::dataToString(data["map"]), "{a: 1, b: 2, c: 3}");
-  EXPECT_EQ(internal::dataToString(data["set"]), "[1.1, 2.2, 3.3]");
-  EXPECT_EQ(internal::dataToString(data["mat"]), "[[1, 0, 0], [0, 1, 0], [0, 0, 1]]");
+  // note: float reformatting should have no effect on other fields (and fixes full precision formatting for internal
+  // yaml representation)
+  EXPECT_EQ(internal::dataToString(data["i"], true), "1");
+  EXPECT_EQ(internal::dataToString(data["f"], true), "2.1");
+  EXPECT_EQ(internal::dataToString(data["d"], true), "3.2");
+  EXPECT_EQ(internal::dataToString(data["b"], true), "true");
+  EXPECT_EQ(internal::dataToString(data["u8"], true), "4");
+  EXPECT_EQ(internal::dataToString(data["s"], true), "test string");
+  EXPECT_EQ(internal::dataToString(data["vec"], true), "[1, 2, 3]");
+  EXPECT_EQ(internal::dataToString(data["map"], true), "{a: 1, b: 2, c: 3}");
+  EXPECT_EQ(internal::dataToString(data["set"], true), "[1.1, 2.2, 3.3]");
+  EXPECT_EQ(internal::dataToString(data["mat"], true), "[[1, 0, 0], [0, 1, 0], [0, 0, 1]]");
   YAML::Node nested_set;
   nested_set["a"]["x"] = 1;
   nested_set["a"]["y"] = 2;
   nested_set["b"]["x"] = 3;
   nested_set["b"]["y"] = 4;
-  EXPECT_EQ(internal::dataToString(nested_set), "{a: {x: 1, y: 2}, b: {x: 3, y: 4}}");
+  EXPECT_EQ(internal::dataToString(nested_set, true), "{a: {x: 1, y: 2}, b: {x: 3, y: 4}}");
 }
 
 TEST(AslFormatter, FormatErrors) {
@@ -222,6 +224,7 @@ TEST(AslFormatter, FormatConfig) {
   Settings().indicate_default_values = false;
   Settings().indicate_units = false;
   Settings().inline_subconfig_field_names = true;
+  Settings().reformat_floats = true;
   std::string formatted = internal::Formatter::formatConfig(data);
   std::string expected =
       R"""(================================= Test Config ==================================
@@ -339,6 +342,7 @@ TEST(AslFormatter, FormatUnits) {
   Settings().indicate_units = true;
   Settings().inline_subconfig_field_names = true;
   Settings().print_width = 80;  // force print width to be consistent for tests
+  Settings().print_indent = 20;
 
   internal::MetaData data = internal::Visitor::getValues(TestConfig());
   const std::string formatted = internal::Formatter::formatConfig(data);
@@ -381,6 +385,7 @@ TEST(AslFormatter, FormatDefaultValues) {
   Settings().indicate_default_values = true;
   Settings().indicate_units = false;
   Settings().inline_subconfig_field_names = true;
+  Settings().print_indent = 20;
 
   const internal::MetaData default_data = internal::Visitor::getValues(TestConfig());
   std::string formatted = internal::Formatter::formatConfig(default_data);
@@ -423,7 +428,7 @@ sub_sub_config [SubSubConfig] (default):
   expected = R"""(================================= Test Config ==================================
 i:                  2
 f:                  -1
-d:                  3.1415926
+d:                  3.14159
 b:                  false
 u8:                 255
 s:                  a different test string

config_utilities/test/tests/config_arrays.cpp

@@ -40,6 +40,7 @@
 #include "config_utilities/formatting/asl.h"
 #include "config_utilities/parsing/yaml.h"
 #include "config_utilities/printing.h"
+#include "config_utilities/settings.h"
 #include "config_utilities/test/utils.h"
 #include "config_utilities/validation.h"
 #include "config_utilities/virtual_config.h"
@@ -340,6 +341,7 @@ TEST(ConfigArrays, PrintArrayConfigs) {
   configs.emplace_back("a", 1.0f);
   configs.emplace_back("b", 2.0f);
   configs.emplace_back("c", 3.0f);
+  Settings().print_indent = 20;
 
   internal::Formatter::setFormatter(std::make_unique<internal::AslFormatter>());
 

config_utilities/test/tests/conversions.cpp

@@ -55,12 +55,12 @@ std::string toYamlString(const T& conf) {
 }
 
 struct ConversionStruct {
-  int num_threads = 1;
+  int num_threads = -1;
   char some_character = 'a';
 };
 
 struct NoConversionStruct {
-  int num_threads = 1;
+  int num_threads = -1;
   uint8_t some_character = 'a';
 };