tests: Move configuration file count to constant

[phlax]

Signed-off-by: Ryan Northey ryan@synca.io

Commit Message: tests: Move configuration file count to constant
Additional Description:

This makes it easier to update/document new sandboxes.

Ideally we set the var dynamically - possibly by setting the constant from an env var - and maybe updating verify_examples.sh to get the expected count of files

One bit of complexity not (yet) dealt with here is that it hardcodes -1 when running on windows or mac - we may want to somehow account for this dynamically too (...update; if its safe to just use the count of bundled yaml file we can remove this decrement)

See #13200 (comment) for discussion with @dio on reasoning for this

Risk Level: low
Testing: yep
Docs Changes:
Release Notes:
[Optional Runtime guard:]
[Optional Fixes #Issue] touch #13200
[Optional Deprecated:]

[phlax]

im thinking we should either move the -1 to whatever counts the files so we dont need this if clause, or we also set the windows/mac decrement somewhere

[lizan]

+1

[dio]

Not sure if automating counting the *.yaml makes sense.

diff --git a/test/config_test/BUILD b/test/config_test/BUILD
index 496cac0e82..792ba3d603 100644
--- a/test/config_test/BUILD
+++ b/test/config_test/BUILD
@@ -24,6 +24,7 @@ envoy_cc_test(
     ],
     deps = [
         ":config_test_lib",
+        "//source/common/filesystem:filesystem_lib",
         "//test/test_common:environment_lib",
         "//test/test_common:utility_lib",
     ],
diff --git a/test/config_test/example_configs_test.cc b/test/config_test/example_configs_test.cc
index 2e02d41608..b430638813 100644
--- a/test/config_test/example_configs_test.cc
+++ b/test/config_test/example_configs_test.cc
@@ -1,6 +1,7 @@
 #include "test/config_test/config_test.h"
 #include "test/test_common/environment.h"
 #include "test/test_common/utility.h"
+#include "common/filesystem/filesystem_impl.h"
 
 #include "gtest/gtest.h"
 
@@ -9,6 +10,15 @@ TEST(ExampleConfigsTest, All) {
   TestEnvironment::exec(
       {TestEnvironment::runfilesPath("test/config_test/example_configs_test_setup.sh")});
 
+#ifdef WIN32
+  Filesystem::InstanceImplWin32 file_system;
+#else
+  Filesystem::InstanceImplPosix file_system;
+#endif
+
+  const auto number_of_configs = std::stoi(
+      file_system.fileReadToEnd(TestEnvironment::temporaryDirectory() + "/number-of-configs.txt"));
+
   // Change working directory, otherwise we won't be able to read files using relative paths.
 #ifdef PATH_MAX
   char cwd[PATH_MAX];
@@ -21,9 +31,9 @@ TEST(ExampleConfigsTest, All) {
 
 #if defined(__APPLE__) || defined(WIN32)
   // freebind/freebind.yaml is not supported on macOS or Windows and is disabled via Bazel.
-  EXPECT_EQ(37UL, ConfigTest::run(directory));
+  EXPECT_EQ(number_of_configs - 1, ConfigTest::run(directory));
 #else
-  EXPECT_EQ(38UL, ConfigTest::run(directory));
+  EXPECT_EQ(number_of_configs, ConfigTest::run(directory));
 #endif
 
   ConfigTest::testMerge();
diff --git a/test/config_test/example_configs_test_setup.sh b/test/config_test/example_configs_test_setup.sh
index 876438e7be..b1478c60e0 100755
--- a/test/config_test/example_configs_test_setup.sh
+++ b/test/config_test/example_configs_test_setup.sh
@@ -5,3 +5,6 @@ set -e
 DIR="$TEST_TMPDIR"/test/config_test
 mkdir -p "$DIR"
 tar -xvf "$TEST_SRCDIR"/envoy/configs/example_configs.tar -C "$DIR"
+
+NUMBER_OF_CONFIGS=$(find "$DIR" -type f | grep .yaml | wc -l)
+echo "$NUMBER_OF_CONFIGS" > "$TEST_TMPDIR"/number-of-configs.txt

[phlax]

Not sure if automating counting the *.yaml makes sense.

hmm - trying to understand - my ~guess is that the expected count is to ensure it loops/tests the correct number of times - so counting the files is ok (as your patch suggests)

ultimately my goal in trying to automate this is so that you dont have to change anything when adding/removing a sandbox, and also no need to document that

[yanavlasov]

@envoyproxy/windows-dev

[phlax]

would it be safe/possible to check the platform in here - and maybe exclude freebind/freebind.yaml or similar, and if necessary - so we can remove some of the if mac/windows... in the test file ?

[phlax]

hmm - maybe it doesnt matter if its just counting the tarred config files - and more explicit having it in the test file.

Im still thinking we should move the decrement to a constant tho - so it is more explicit

[phlax]

if im not mistaken - the freebind.yaml file is already excluded from config tar - which basically means we can just remove the decrement altogether - if its safe to use the tarred file count

[dio]

Can we use /usr/bin/env's help? Not sure about Windows 😅.

[phlax]

not sure how that would work on windows - just waiting to see if the full path fixes the problem...

[phlax]

full path fixed the find issue

a quick search of "windows /usr/bin/env find" doesnt seem to suggest that would work (or otherwise really) - i think probably just using the full path should be safe/r

[sunjayBhatia]

Yeah using #!/usr/bin/env bash, /c/Windows/System32 shows up on the PATH first still, to get around that we would have to do some PATH ordering magic somewhere

[dio]

So we are testing freebind on all platforms now?

[sunjayBhatia]

I think it is excluded via Bazel select in the filegroup that collects configs

[sunjayBhatia]

envoy/configs/BUILD

Lines 30 to 34 in c8b894c

	] + select({
	"//bazel:apple": [],
	"//bazel:windows_x86_64": [],
	"//conditions:default": ["freebind/freebind.yaml"],
	}),

[dio]

Gotcha. Sorry for the noise.

[dio]

Thanks!

[dio]

@phlax sorry, could you update this PR desc and commit message to reflect the actual change? Thanks!