Fix 4 test failures due to broken CRLF line handling (envoyproxy#12381)

- test/test_common/environment.cc was reading config files for substitution in binary (preserving \r) and then writing the substituted resulting file in text (replacing \n with \r\n), ending up with \r\r\n line endings. - //test/common/runtime:runtime_impl_test now generates multiline strings on the fly to ensure line endings are treated in string as a binary blob Co-authored-by: Sunjay Bhatia <[email protected]> Co-authored-by: William A Rowe Jr <[email protected]> Signed-off-by: Sunjay Bhatia <[email protected]> Signed-off-by: William A Rowe Jr <[email protected]> Signed-off-by: chaoqinli <[email protected]>
chaoqin-li1123 · Aug 7, 2020 · 34e929f · 34e929f
1 parent 3413777
commit 34e929f
Show file tree

Hide file tree

Showing 8 changed files with 16 additions and 16 deletions.
diff --git a/test/common/buffer/BUILD b/test/common/buffer/BUILD
@@ -74,8 +74,6 @@ envoy_cc_test(
 envoy_cc_test(
     name = "watermark_buffer_test",
     srcs = ["watermark_buffer_test.cc"],
-    # Fails on windows with cr/lf yaml file checkouts
-    tags = ["fails_on_windows"],
     deps = [
         ":utility_lib",
         "//source/common/buffer:buffer_lib",

diff --git a/test/common/common/BUILD b/test/common/common/BUILD
@@ -181,8 +181,6 @@ envoy_cc_test(
 envoy_cc_test(
     name = "random_generator_test",
     srcs = ["random_generator_test.cc"],
-    # Fails on windows with cr/lf yaml file checkouts
-    tags = ["fails_on_windows"],
     deps = [
         "//source/common/common:random_generator_lib",
         "//test/mocks/runtime:runtime_mocks",

diff --git a/test/common/runtime/BUILD b/test/common/runtime/BUILD
@@ -42,8 +42,6 @@ envoy_cc_test(
     name = "runtime_impl_test",
     srcs = ["runtime_impl_test.cc"],
     data = glob(["test_data/**"]) + ["filesystem_setup.sh"],
-    # Fails on windows with cr/lf yaml file checkouts
-    tags = ["fails_on_windows"],
     deps = [
         "//source/common/config:runtime_utility_lib",
         "//source/common/runtime:runtime_lib",

diff --git a/test/common/runtime/filesystem_setup.sh b/test/common/runtime/filesystem_setup.sh
@@ -9,6 +9,9 @@ cd "${TEST_SRCDIR}/envoy"
 rm -rf "${TEST_TMPDIR}/${TEST_DATA}"
 mkdir -p "${TEST_TMPDIR}/${TEST_DATA}"
 cp -RfL "${TEST_DATA}"/* "${TEST_TMPDIR}/${TEST_DATA}"
+# Verify text value is treated as a binary blob regardless of source line-ending settings
+printf "hello\nworld" > "${TEST_TMPDIR}/${TEST_DATA}/root/envoy/file_lf"
+printf "hello\r\nworld" > "${TEST_TMPDIR}/${TEST_DATA}/root/envoy/file_crlf"
 chmod -R u+rwX "${TEST_TMPDIR}/${TEST_DATA}"
 
 # Deliberate symlink of doom.

diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc
@@ -140,12 +140,14 @@ TEST_F(DiskLoaderImplTest, All) {
 
   // Basic string getting.
   EXPECT_EQ("world", loader_->snapshot().get("file2").value().get());
-  EXPECT_EQ("hello\nworld", loader_->snapshot().get("subdir.file3").value().get());
+  EXPECT_EQ("hello", loader_->snapshot().get("subdir.file").value().get());
+  EXPECT_EQ("hello\nworld", loader_->snapshot().get("file_lf").value().get());
+  EXPECT_EQ("hello\r\nworld", loader_->snapshot().get("file_crlf").value().get());
   EXPECT_FALSE(loader_->snapshot().get("invalid").has_value());
 
   // Existence checking.
   EXPECT_EQ(true, loader_->snapshot().get("file2").has_value());
-  EXPECT_EQ(true, loader_->snapshot().get("subdir.file3").has_value());
+  EXPECT_EQ(true, loader_->snapshot().get("subdir.file").has_value());
   EXPECT_EQ(false, loader_->snapshot().get("invalid").has_value());
 
   // Integer getting.
@@ -255,7 +257,7 @@ TEST_F(DiskLoaderImplTest, All) {
 
   EXPECT_EQ(0, store_.counter("runtime.load_error").value());
   EXPECT_EQ(1, store_.counter("runtime.load_success").value());
-  EXPECT_EQ(23, store_.gauge("runtime.num_keys", Stats::Gauge::ImportMode::NeverImport).value());
+  EXPECT_EQ(25, store_.gauge("runtime.num_keys", Stats::Gauge::ImportMode::NeverImport).value());
   EXPECT_EQ(4, store_.gauge("runtime.num_layers", Stats::Gauge::ImportMode::NeverImport).value());
 }
 
@@ -556,7 +558,7 @@ TEST_F(StaticLoaderImplTest, ProtoParsing) {
     file12: FaLSe
     file13: false
     subdir:
-      file3: "hello\nworld"
+      file: "hello"
     numerator_only:
       numerator: 52
     denominator_only:
@@ -567,14 +569,18 @@ TEST_F(StaticLoaderImplTest, ProtoParsing) {
     empty: {}
     file_with_words: "some words"
     file_with_double: 23.2
+    file_lf: "hello\nworld"
+    file_crlf: "hello\r\nworld"
     bool_as_int0: 0
     bool_as_int1: 1
   )EOF");
   setup();
 
   // Basic string getting.
   EXPECT_EQ("world", loader_->snapshot().get("file2").value().get());
-  EXPECT_EQ("hello\nworld", loader_->snapshot().get("subdir.file3").value().get());
+  EXPECT_EQ("hello", loader_->snapshot().get("subdir.file").value().get());
+  EXPECT_EQ("hello\nworld", loader_->snapshot().get("file_lf").value().get());
+  EXPECT_EQ("hello\r\nworld", loader_->snapshot().get("file_crlf").value().get());
   EXPECT_FALSE(loader_->snapshot().get("invalid").has_value());
 
   // Integer getting.
@@ -674,7 +680,7 @@ TEST_F(StaticLoaderImplTest, ProtoParsing) {
 
   EXPECT_EQ(0, store_.counter("runtime.load_error").value());
   EXPECT_EQ(1, store_.counter("runtime.load_success").value());
-  EXPECT_EQ(19, store_.gauge("runtime.num_keys", Stats::Gauge::ImportMode::NeverImport).value());
+  EXPECT_EQ(21, store_.gauge("runtime.num_keys", Stats::Gauge::ImportMode::NeverImport).value());
   EXPECT_EQ(2, store_.gauge("runtime.num_layers", Stats::Gauge::ImportMode::NeverImport).value());
 }
 

diff --git a/...runtime/test_data/root/envoy/subdir/file3 → .../runtime/test_data/root/envoy/subdir/file b/...runtime/test_data/root/envoy/subdir/file3 → .../runtime/test_data/root/envoy/subdir/file
@@ -1,2 +1 @@
 hello
-world
diff --git a/test/exe/BUILD b/test/exe/BUILD
@@ -63,8 +63,6 @@ envoy_cc_test(
     name = "main_common_test",
     srcs = ["main_common_test.cc"],
     data = ["//test/config/integration:google_com_proxy_port_0"],
-    # Fails on windows with cr/lf yaml file checkouts
-    tags = ["fails_on_windows"],
     deps = [
         "//source/common/api:api_lib",
         "//source/exe:main_common_lib",

diff --git a/test/test_common/environment.cc b/test/test_common/environment.cc
@@ -394,7 +394,7 @@ std::string TestEnvironment::temporaryFileSubstitute(const std::string& path,
   const std::string out_json_path =
       TestEnvironment::temporaryPath(name) + ".with.ports" + extension;
   {
-    std::ofstream out_json_file(out_json_path);
+    std::ofstream out_json_file(out_json_path, std::ios::binary);
     out_json_file << out_json_string;
   }
   return out_json_path;