From 6136bc270e77d8b7d4e6b63aa9265eae812b9335 Mon Sep 17 00:00:00 2001
From: Steve Pham <82231385+spham-amzn@users.noreply.github.com>
Date: Tue, 25 May 2021 16:05:40 -0700
Subject: [PATCH] Remove flaky test from AzNetwork instead of using retry

- Remove '--repeat until-pass' from profile test ctest argument
- Moved flaky TCP tests from main googletest suite to sandbox
- Added 'TARGET' to 'ly_add_googletest' to support adding the same module to multiple tests or adding a test that is not named the same as the module
- Fix minor bug in ly_add_googletest
---
 Code/Framework/AzNetworking/CMakeLists.txt    |  7 +++++
 .../Tests/TcpTransport/TcpTransportTests.cpp  |  4 +--
 cmake/LYTestWrappers.cmake                    | 17 ++++++++----
 .../build/Platform/Linux/build_config.json    | 27 ++++++++++++++++---
 4 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/Code/Framework/AzNetworking/CMakeLists.txt b/Code/Framework/AzNetworking/CMakeLists.txt
index 0fe95441ce..c6673058d7 100644
--- a/Code/Framework/AzNetworking/CMakeLists.txt
+++ b/Code/Framework/AzNetworking/CMakeLists.txt
@@ -65,5 +65,12 @@ if(PAL_TRAIT_BUILD_TESTS_SUPPORTED)
     ly_add_googletest(
         NAME AZ::AzNetworking.Tests
     )
+
+    ly_add_googletest(
+        NAME AZ::AzNetworking.Tests.Sandbox
+        TARGET AZ::AzNetworking.Tests
+        TEST_SUITE sandbox
+    )
+    
 endif()
 
diff --git a/Code/Framework/AzNetworking/Tests/TcpTransport/TcpTransportTests.cpp b/Code/Framework/AzNetworking/Tests/TcpTransport/TcpTransportTests.cpp
index eed12e1881..7cc3af5a51 100644
--- a/Code/Framework/AzNetworking/Tests/TcpTransport/TcpTransportTests.cpp
+++ b/Code/Framework/AzNetworking/Tests/TcpTransport/TcpTransportTests.cpp
@@ -129,7 +129,7 @@ namespace UnitTest
     #if AZ_TRAIT_DISABLE_FAILED_NETWORKING_TESTS
     TEST_F(TcpTransportTests, DISABLED_TestSingleClient)
     #else
-    TEST_F(TcpTransportTests, TestSingleClient)
+    TEST_F(TcpTransportTests, SUITE_sandbox_TestSingleClient)
     #endif // AZ_TRAIT_DISABLE_FAILED_NETWORKING_TESTS
     {
         TestTcpServer testServer;
@@ -157,7 +157,7 @@ namespace UnitTest
     #if AZ_TRAIT_DISABLE_FAILED_NETWORKING_TESTS
     TEST_F(TcpTransportTests, DISABLED_TestMultipleClients)
     #else
-    TEST_F(TcpTransportTests, TestMultipleClients)
+    TEST_F(TcpTransportTests, SUITE_sandbox_TestMultipleClients)
     #endif // AZ_TRAIT_DISABLE_FAILED_NETWORKING_TESTS
     {
         constexpr uint32_t NumTestClients = 50;
diff --git a/cmake/LYTestWrappers.cmake b/cmake/LYTestWrappers.cmake
index 2a65929cf6..b4d6fe308e 100644
--- a/cmake/LYTestWrappers.cmake
+++ b/cmake/LYTestWrappers.cmake
@@ -370,6 +370,7 @@ endfunction()
 #! ly_add_googletest: Adds a new RUN_TEST using for the specified target using the supplied command or fallback to running
 #                     googletest tests through AzTestRunner
 # \arg:NAME Name to for the test run target
+# \arg:TARGET Name of the target module that is being run for tests. If not provided, will default to 'NAME'
 # \arg:TEST_REQUIRES(optional) List of system resources that are required to run this test.
 #      Only available option is "gpu"
 # \arg:TEST_SUITE(optional) - "smoke" or "periodic" or "sandbox" - prevents the test from running normally
@@ -384,14 +385,20 @@ function(ly_add_googletest)
         message(FATAL_ERROR "Platform does not support test targets")
     endif()
 
-    set(one_value_args NAME TEST_SUITE) 
+    set(one_value_args NAME TARGET TEST_SUITE) 
     set(multi_value_args TEST_COMMAND COMPONENT)
     cmake_parse_arguments(ly_add_googletest "${options}" "${one_value_args}" "${multi_value_args}" ${ARGN})
 
+    if (ly_add_googletest_TARGET)
+        set(target_name ${ly_add_googletest_TARGET})
+    else()
+        set(target_name ${ly_add_googletest_NAME})
+    endif()
+
 
     # AzTestRunner modules only supports google test libraries, regardless of whether or not 
     # google test suites are supported
-    set_property(GLOBAL APPEND PROPERTY LY_AZTESTRUNNER_TEST_MODULES "${ly_add_googletest_NAME}")
+    set_property(GLOBAL APPEND PROPERTY LY_AZTESTRUNNER_TEST_MODULES "${target_name}")
 
     if(NOT PAL_TRAIT_TEST_GOOGLE_TEST_SUPPORTED)
         return()
@@ -400,7 +407,7 @@ function(ly_add_googletest)
 
     if (ly_add_googletest_TEST_SUITE AND NOT ly_add_googletest_TEST_SUITE STREQUAL "main")
         # if a suite is specified, we filter to only accept things which match that suite (in c++)
-        set(non_ide_params "-gtest_filter=*SUITE_${ly_add_googletest_TEST_SUITE}*")
+        set(non_ide_params "--gtest_filter=*SUITE_${ly_add_googletest_TEST_SUITE}*")
     else()
         # otherwise, if its the main suite we only runs things that dont have any of the other suites. 
         # Note: it doesn't do AND, only 'or' - so specifying SUITE_main:REQUIRES_gpu
@@ -412,11 +419,11 @@ function(ly_add_googletest)
 
     if(NOT ly_add_googletest_TEST_COMMAND)
         # Use the NAME parameter as the build target
-        set(build_target ${ly_add_googletest_NAME})
+        set(build_target ${target_name})
         ly_strip_target_namespace(TARGET ${build_target} OUTPUT_VARIABLE build_target)
 
         if(NOT TARGET ${build_target})
-            message(FATAL_ERROR "A valid build target \"${build_target}\" for test run \"${ly_add_googletest_NAME}\" has not been found.\
+            message(FATAL_ERROR "A valid build target \"${build_target}\" for test run \"${target_name}\" has not been found.\
                 A valid target via the TARGET parameter or a custom TEST_COMMAND must be supplied")
         endif()
 
diff --git a/scripts/build/Platform/Linux/build_config.json b/scripts/build/Platform/Linux/build_config.json
index 5d96ae7846..c1646fc863 100644
--- a/scripts/build/Platform/Linux/build_config.json
+++ b/scripts/build/Platform/Linux/build_config.json
@@ -83,7 +83,7 @@
       "CMAKE_OPTIONS": "-G 'Ninja Multi-Config' -DCMAKE_C_COMPILER=clang-6.0 -DCMAKE_CXX_COMPILER=clang++-6.0 -DLY_UNITY_BUILD=TRUE -DLY_PARALLEL_LINK_JOBS=4 -DO3DE_HOME_PATH=\"${WORKSPACE}/home\" -DO3DE_REGISTER_ENGINE_PATH=\"${WORKSPACE}/o3de\" -DO3DE_REGISTER_THIS_ENGINE=TRUE",
       "CMAKE_LY_PROJECTS": "AutomatedTesting",
       "CMAKE_TARGET": "all",
-      "CTEST_OPTIONS": "-E (Gem::EMotionFX.Editor.Tests|Gem::AWSClientAuth.Tests|Gem::AWSCore.Editor.Tests) -L FRAMEWORK_googletest --repeat until-pass:5"
+      "CTEST_OPTIONS": "-E (Gem::EMotionFX.Editor.Tests|Gem::AWSClientAuth.Tests|Gem::AWSCore.Editor.Tests) -L FRAMEWORK_googletest"
     }
   },
   "test_profile_nounity": {
@@ -95,7 +95,7 @@
       "CMAKE_OPTIONS": "-G 'Ninja Multi-Config' -DCMAKE_C_COMPILER=clang-6.0 -DCMAKE_CXX_COMPILER=clang++-6.0 -DLY_UNITY_BUILD=FALSE -DLY_PARALLEL_LINK_JOBS=4 -DO3DE_HOME_PATH=\"${WORKSPACE}/home\" -DO3DE_REGISTER_ENGINE_PATH=\"${WORKSPACE}/o3de\" -DO3DE_REGISTER_THIS_ENGINE=TRUE",
       "CMAKE_LY_PROJECTS": "AutomatedTesting",
       "CMAKE_TARGET": "all",
-      "CTEST_OPTIONS": "-E (Gem::EMotionFX.Editor.Tests|Gem::AWSClientAuth.Tests|Gem::AWSCore.Editor.Tests) -L FRAMEWORK_googletest --repeat until-pass:5"
+      "CTEST_OPTIONS": "-E (Gem::EMotionFX.Editor.Tests|Gem::AWSClientAuth.Tests|Gem::AWSCore.Editor.Tests) -L FRAMEWORK_googletest"
     }
   },
   "asset_profile": {
@@ -143,7 +143,26 @@
       "CMAKE_OPTIONS": "-G 'Ninja Multi-Config' -DCMAKE_C_COMPILER=clang-6.0 -DCMAKE_CXX_COMPILER=clang++-6.0 -DLY_UNITY_BUILD=TRUE -DLY_PARALLEL_LINK_JOBS=4 -DO3DE_HOME_PATH=\"${WORKSPACE}/home\" -DO3DE_REGISTER_ENGINE_PATH=\"${WORKSPACE}/o3de\" -DO3DE_REGISTER_THIS_ENGINE=TRUE",
       "CMAKE_LY_PROJECTS": "AutomatedTesting",
       "CMAKE_TARGET": "TEST_SUITE_periodic",
-      "CTEST_OPTIONS": "-L \"(SUITE_periodic)\""
+      "CTEST_OPTIONS": "-L (SUITE_periodic)"
+    }
+  },
+  "sandbox_test_profile": {
+    "TAGS": [
+      "nightly-incremental",
+      "nightly-clean",
+      "weekly-build-metrics"
+    ],
+    "PIPELINE_ENV": {
+      "ON_FAILURE_MARK": "UNSTABLE"
+    },
+    "COMMAND": "build_test_linux.sh",
+    "PARAMETERS": {
+      "CONFIGURATION": "profile",
+      "OUTPUT_DIRECTORY": "build/linux",
+      "CMAKE_OPTIONS": "-G 'Ninja Multi-Config' -DCMAKE_C_COMPILER=clang-6.0 -DCMAKE_CXX_COMPILER=clang++-6.0 -DLY_UNITY_BUILD=TRUE -DLY_PARALLEL_LINK_JOBS=4 -DO3DE_HOME_PATH=\"${WORKSPACE}/home\" -DO3DE_REGISTER_ENGINE_PATH=\"${WORKSPACE}/o3de\" -DO3DE_REGISTER_THIS_ENGINE=TRUE",
+      "CMAKE_LY_PROJECTS": "AutomatedTesting",
+      "CMAKE_TARGET": "all",
+      "CTEST_OPTIONS": "-L (SUITE_sandbox)"
     }
   },
   "benchmark_test_profile": {
@@ -159,7 +178,7 @@
       "CMAKE_OPTIONS": "-G 'Ninja Multi-Config' -DCMAKE_C_COMPILER=clang-6.0 -DCMAKE_CXX_COMPILER=clang++-6.0 -DLY_UNITY_BUILD=TRUE -DLY_PARALLEL_LINK_JOBS=4 -DO3DE_HOME_PATH=\"${WORKSPACE}/home\" -DO3DE_REGISTER_ENGINE_PATH=\"${WORKSPACE}/o3de\" -DO3DE_REGISTER_THIS_ENGINE=TRUE",
       "CMAKE_LY_PROJECTS": "AutomatedTesting",
       "CMAKE_TARGET": "TEST_SUITE_benchmark",
-      "CTEST_OPTIONS": "-L \"(SUITE_benchmark)\""
+      "CTEST_OPTIONS": "-L (SUITE_benchmark)"
     }
   },
   "release": {