From be83e1ed60d3a34fc98b69b88de83b233502a618 Mon Sep 17 00:00:00 2001
From: David Sisson <EpsilonPrime@users.noreply.github.com>
Date: Thu, 10 Aug 2023 18:32:17 -0700
Subject: [PATCH 1/2] feat: enable roundtrip testing of textplan conversion

---
 src/substrait/textplan/tests/CMakeLists.txt   | 80 ++++++++-----------
 .../textplan/tests/RoundtripTest.cpp          | 12 +--
 2 files changed, 41 insertions(+), 51 deletions(-)

diff --git a/src/substrait/textplan/tests/CMakeLists.txt b/src/substrait/textplan/tests/CMakeLists.txt
index 24be6792..97bd91be 100644
--- a/src/substrait/textplan/tests/CMakeLists.txt
+++ b/src/substrait/textplan/tests/CMakeLists.txt
@@ -19,50 +19,38 @@ add_test_case(
   gtest
   gtest_main)
 
-option(SUBSTRAIT_CPP_ROUNDTRIP_TESTING
-       "Enable substrait-cpp textplan roundtrip tests." OFF)
-
-if(${SUBSTRAIT_CPP_ROUNDTRIP_TESTING})
-  add_test_case(
-    round_trip_test
-    SOURCES
-    RoundtripTest.cpp
-    EXTRA_LINK_LIBS
-    substrait_textplan_converter
-    substrait_textplan_loader
-    substrait_textplan_normalizer
-    substrait_common
-    substrait_proto
-    parse_result_matchers
-    protobuf-matchers
-    fmt::fmt-header-only
-    gmock
-    gtest
-    gtest_main)
-
-  cmake_path(GET CMAKE_CURRENT_SOURCE_DIR PARENT_PATH TEXTPLAN_SOURCE_DIR)
-
-  add_custom_command(
-    TARGET round_trip_test
-    POST_BUILD
-    COMMAND ${CMAKE_COMMAND} -E echo "Copying unit test data.."
-    COMMAND ${CMAKE_COMMAND} -E make_directory
-            "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data"
-    COMMAND
-      ${CMAKE_COMMAND} -E copy
-      "${TEXTPLAN_SOURCE_DIR}/converter/data/q6_first_stage.json"
-      "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data/q6_first_stage.json"
-    COMMAND ${CMAKE_COMMAND} -E copy "${TEXTPLAN_SOURCE_DIR}/data/*.json"
-            "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data/")
-
-  message(
-    STATUS
-      "test data will be here:  ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data")
-else()
-
-  message(
-    STATUS
-      "Round trip testing is turned off.  Add SUBSTRAIT_CPP_ROUNDTRIP_TESTING=on to enable."
-  )
+add_test_case(
+  substrait_textplan_round_trip_test
+  SOURCES
+  RoundtripTest.cpp
+  EXTRA_LINK_LIBS
+  substrait_textplan_converter
+  substrait_textplan_loader
+  substrait_textplan_normalizer
+  substrait_common
+  substrait_proto
+  parse_result_matchers
+  protobuf-matchers
+  fmt::fmt-header-only
+  gmock
+  gtest
+  gtest_main)
 
-endif()
+cmake_path(GET CMAKE_CURRENT_SOURCE_DIR PARENT_PATH TEXTPLAN_SOURCE_DIR)
+
+add_custom_command(
+  TARGET substrait_textplan_round_trip_test
+  POST_BUILD
+  COMMAND ${CMAKE_COMMAND} -E echo "Copying unit test data.."
+  COMMAND ${CMAKE_COMMAND} -E make_directory
+          "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data"
+  COMMAND
+    ${CMAKE_COMMAND} -E copy
+    "${TEXTPLAN_SOURCE_DIR}/converter/data/q6_first_stage.json"
+    "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data/q6_first_stage.json"
+  COMMAND ${CMAKE_COMMAND} -E copy "${TEXTPLAN_SOURCE_DIR}/data/*.json"
+          "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data/")
+
+message(
+  STATUS
+    "test data will be here:  ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data")
diff --git a/src/substrait/textplan/tests/RoundtripTest.cpp b/src/substrait/textplan/tests/RoundtripTest.cpp
index 3b22071c..7059b934 100644
--- a/src/substrait/textplan/tests/RoundtripTest.cpp
+++ b/src/substrait/textplan/tests/RoundtripTest.cpp
@@ -19,8 +19,7 @@
 #include "substrait/textplan/tests/ParseResultMatchers.h"
 
 using ::protobuf_matchers::EqualsProto;
-using ::protobuf_matchers::IgnoringFieldPaths;
-using ::protobuf_matchers::Partially;
+using ::protobuf_matchers::IgnoringFields;
 using ::testing::AllOf;
 
 namespace io::substrait::textplan {
@@ -98,7 +97,11 @@ TEST_P(RoundTripBinaryToTextFixture, RoundTrip) {
   ASSERT_THAT(
       result,
       ::testing::AllOf(
-          ParsesOk(), HasErrors({}), AsBinaryPlan(EqualsProto(normalizedPlan))))
+          ParsesOk(),
+          HasErrors({}),
+          AsBinaryPlan(IgnoringFields(
+              {"substrait.proto.RelCommon.Emit.output_mapping"},
+              EqualsProto(normalizedPlan)))))
       << std::endl
       << "Intermediate result:" << std::endl
       << addLineNumbers(outputText) << std::endl
@@ -115,8 +118,7 @@ INSTANTIATE_TEST_SUITE_P(
       if (lastSlash != std::string::npos) {
         identifier = identifier.substr(lastSlash);
       }
-      if (identifier.length() > 5 &&
-          identifier.substr(identifier.length() - 5) == ".json") {
+      if (endsWith(identifier, ".json")) {
         identifier = identifier.substr(0, identifier.length() - 5);
       }
 

From 67a271c1614c6b00f044cbbe661648143df87263 Mon Sep 17 00:00:00 2001
From: David Sisson <EpsilonPrime@users.noreply.github.com>
Date: Thu, 10 Aug 2023 18:43:56 -0700
Subject: [PATCH 2/2] Ran clang format.

---
 src/substrait/textplan/tests/CMakeLists.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/substrait/textplan/tests/CMakeLists.txt b/src/substrait/textplan/tests/CMakeLists.txt
index 97bd91be..116ec4e7 100644
--- a/src/substrait/textplan/tests/CMakeLists.txt
+++ b/src/substrait/textplan/tests/CMakeLists.txt
@@ -52,5 +52,5 @@ add_custom_command(
           "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data/")
 
 message(
-  STATUS
-    "test data will be here:  ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data")
+  STATUS "test data will be here:  ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data"
+)