Repository: microsoft/terminal (language: cpp)

Issue:
Handling of Skipped Tests in Helix framework
We have been using the Skipped state for two situations in our code:
1. The test is dynamically deciding it cannot run because of situations only detectible at runtime in the operating system
2. The test is broken or flaky for some reason and we want to turn it off temporarily until we can fix it in a work item.

Unfortunately, with the adoption of the Helix scripts from Microsoft-UI-XAML, this isn't fully compatible.
This is because the Helix scripts, when run on the Helix machines, have some retry logic in them. They take anything that is NOT a pass and they re-run it up to 10 times. They store all the subresults and bucket tests into 3 categories:
1. Pass - Just flat out passed
2. Skipped - Flaky/unreliable, had some mix of pass/fail on the runs
3. Fail - Just flat out failed.

Then in the final post-processing steps for test results, things that are Skipped are evaluated for the pass/fail threshold and converted to either Pass or Fail based on whether they indicated "Pass" above or below the threshold count.

This makes our usage of Skipped do the following:
1. The scripts see the 'Skipped' out of TAEF as !Passed and mark them for rerun.
2. They rerun the 'Skipped' test up to 10 times and see !Passed and mark each run as a Fail.
3. The whole flakiness is reported as 'Skipped' temporarily on the build machine
4. In the post processing steps, the subruns of 'Skipped' are evaluated, every one of them is !Passed (and thus Fail) and the test is marked as Fail.

So in summary, the scripts right now only understand two things: Pass and Fail. Also they reuse Skipped as an intermediate counter state only to further evaluate it into Pass/Fail later.

That doesn't work for us.

To complete the PR where I brought Helix online, I used the TestAttribute "Ignore" to "True" instead on tests that need to be disabled temporarily for some reason. I did this only on the UIA/LocalTests because they're the only ones affected by running inside Helix with the retry scripts. The Unit/Feature tests that run on the build machine (and use a small subset of the scripts just to report their test results to AzDO) are fine reporting Skipped as they are.

On talking with the Microsoft UI XAML team, for handling tests that dynamically decide they're OK, they instead chose to report "Pass" and exit early as necessary.

Overall, I think this is what we need to do going forward:
1. For tests dynamically deciding we need to either
   - Convert them to report Pass early and accept that.
   - Teach HelixTestHelpers.cs and the retry logic how to handle either the Skipped or NotRun state (and update the relevant tests as appropriate) to pass that all the way through and report up to AzDO without rerunning them when it's intentionally skipped (and without post-processing them into a binary Pass/Fail state.)
2. For tests that are broken or flaky and need turning off with a filed work item, just set Ignore=True as the test attribute. (This also allows the `te.exe` command-line argument `/runIgnoredTests` to be used to run them when attempting to fix them up.)



Discussion / hints:
See also #7281 and #7282 for disabled tests as well as #6992 for the enablement of Helix

Fix patch:
diff --git a/build/Helix/ConvertWttLogToXUnit.ps1 b/build/Helix/ConvertWttLogToXUnit.ps1
index 13d333d3e75..c17d86e8ec1 100644
--- a/build/Helix/ConvertWttLogToXUnit.ps1
+++ b/build/Helix/ConvertWttLogToXUnit.ps1
@@ -20,9 +20,7 @@ Param(
 $helixResultsContainerUri = $Env:HELIX_RESULTS_CONTAINER_URI
 $helixResultsContainerRsas = $Env:HELIX_RESULTS_CONTAINER_RSAS
 
-$rerunPassesRequiredToAvoidFailure = $env:rerunPassesRequiredToAvoidFailure
-
 Add-Type -Language CSharp -ReferencedAssemblies System.Xml,System.Xml.Linq,System.Runtime.Serialization,System.Runtime.Serialization.Json (Get-Content $PSScriptRoot\HelixTestHelpers.cs -Raw)
 
 $testResultParser = [HelixTestHelpers.TestResultParser]::new($TestNamePrefix, $helixResultsContainerUri, $helixResultsContainerRsas)
-$testResultParser.ConvertWttLogToXUnitLog($WttInputPath, $WttSingleRerunInputPath, $WttMultipleRerunInputPath, $XUnitOutputPath, $rerunPassesRequiredToAvoidFailure)
\ No newline at end of file
+$testResultParser.ConvertWttLogToXUnitLog($WttInputPath, $WttSingleRerunInputPath, $WttMultipleRerunInputPath, $XUnitOutputPath)
diff --git a/build/Helix/HelixTestHelpers.cs b/build/Helix/HelixTestHelpers.cs
index d691026a410..67f13937c08 100644
--- a/build/Helix/HelixTestHelpers.cs
+++ b/build/Helix/HelixTestHelpers.cs
@@ -20,32 +20,13 @@ public TestResult()
         public string Name { get; set; }
         public string SourceWttFile { get; set; }
         public bool Passed { get; set; }
+        public bool Skipped { get; set; }
         public bool CleanupPassed { get; set; }
         public TimeSpan ExecutionTime { get; set; }
         public string Details { get; set; }
 
         public List<string> Screenshots { get; private set; }
         public List<TestResult> RerunResults { get; private set; }
-
-        // Returns true if the test pass rate is sufficient to avoid being counted as a failure.
-        public bool PassedOrUnreliable(int requiredNumberOfPasses)
-        {
-            if(Passed)
-            {
-                return true;
-            }
-            else
-            {
-                if(RerunResults.Count == 1)
-                {
-                    return RerunResults[0].Passed;
-                }
-                else
-                {
-                    return RerunResults.Where(r => r.Passed).Count() >= requiredNumberOfPasses;
-                }
-            }
-        }
     }
     
     //
@@ -221,7 +202,9 @@ public static TestPass ParseTestWttFile(string fileName, bool cleanupFailuresAre
                         testsExecuting--;
 
                         // If any inner test fails, we'll still fail the outer
-                        currentResult.Passed &= element.Attribute("Result").Value == "Pass";
+                        var value = element.Attribute("Result").Value;
+                        currentResult.Passed = value == "Pass";
+                        currentResult.Skipped = value == "Skipped";
 
                         // Only gather execution data if this is the outer test we ran initially
                         if (testsExecuting == 0)
@@ -498,7 +481,7 @@ public Dictionary<string, string> GetSubResultsJsonByMethodName(string wttInputP
             return subResultsJsonByMethod;
         }
 
-        public void ConvertWttLogToXUnitLog(string wttInputPath, string wttSingleRerunInputPath, string wttMultipleRerunInputPath, string xunitOutputPath, int requiredPassRateThreshold)
+        public void ConvertWttLogToXUnitLog(string wttInputPath, string wttSingleRerunInputPath, string wttMultipleRerunInputPath, string xunitOutputPath)
         {
             TestPass testPass = TestPass.ParseTestWttFileWithReruns(wttInputPath, wttSingleRerunInputPath, wttMultipleRerunInputPath, cleanupFailuresAreRegressions: true, truncateTestNames: false);
             var results = testPass.TestResults;
@@ -510,8 +493,8 @@ public void ConvertWttLogToXUnitLog(string wttInputPath, string wttSingleRerunIn
             // If the test failed sufficiently often enough for it to count as a failed test (determined by a property on the
             // Azure DevOps job), we'll later mark it as failed during test results processing.
 
-            int failedCount = results.Where(r => !r.PassedOrUnreliable(requiredPassRateThreshold)).Count();
-            int skippedCount = results.Where(r => !r.Passed && r.PassedOrUnreliable(requiredPassRateThreshold)).Count();
+            int failedCount = results.Where(r => !r.Passed).Count();
+            int skippedCount = results.Where(r => (!r.Passed && r.Skipped)).Count();
 
             var root = new XElement("assemblies");
 
@@ -557,12 +540,13 @@ public void ConvertWttLogToXUnitLog(string wttInputPath, string wttSingleRerunIn
                 
                 string resultString = string.Empty;
                 
-                if (result.Passed)
+                if (result.Passed && !result.Skipped)
                 {
                     resultString = "Pass";
                 }
-                else if(result.PassedOrUnreliable(requiredPassRateThreshold))
+                else if (result.Skipped)
                 {
+
                     resultString = "Skip";
                 }
                 else
@@ -571,31 +555,25 @@ public void ConvertWttLogToXUnitLog(string wttInputPath, string wttSingleRerunIn
                 }
 
                 
-                test.SetAttributeValue("result", resultString);
-
                 if (!result.Passed)
                 {
-                    // If a test failed, we'll have rerun it multiple times.
-                    // We'll save the subresults to a JSON text file that we'll upload to the helix results container -
-                    // this allows it to be as long as we want, whereas the reason field in Azure DevOps has a 4000 character limit.
-                    string subResultsFileName = methodName + "_subresults.json";
-                    string subResultsFilePath = Path.Combine(Path.GetDirectoryName(wttInputPath), subResultsFileName);
-					
-                    if (result.PassedOrUnreliable(requiredPassRateThreshold))
+                    if (result.Skipped)
                     {
                         var reason = new XElement("reason");
-                        reason.Add(new XCData(GetUploadedFileUrl(subResultsFileName, helixResultsContainerUri, helixResultsContainerRsas)));
+                        reason.Add(new XCData("Test skipped"));
                         test.Add(reason);
                     }
-                    else
-                    {
+                    else {
                         var failure = new XElement("failure");
                         var message = new XElement("message");
-						message.Add(new XCData(GetUploadedFileUrl(subResultsFileName, helixResultsContainerUri, helixResultsContainerRsas)));
+                        message.Add(new XCData("Test failed"));
                         failure.Add(message);
                         test.Add(failure);
                     }
                 }
+
+                test.SetAttributeValue("result", resultString);
+
                 collection.Add(test);
             }
 
diff --git a/build/Helix/OutputTestErrorsForAzureDevops.ps1 b/build/Helix/OutputTestErrorsForAzureDevops.ps1
new file mode 100644
index 00000000000..863db32afe7
--- /dev/null
+++ b/build/Helix/OutputTestErrorsForAzureDevops.ps1
@@ -0,0 +1,38 @@
+Param(
+    [Parameter(Mandatory = $true)]
+    [string]$XUnitOutputPath
+)
+
+# This script is used to parse the XUnit output from the test runs and print out
+# the tests that failed.
+#
+# Why you might ask? Well, it sure seems like Azure DevOps doesn't like the fact
+# that we just call our tests in a powershell script. It can't seemingly find
+# the actual errors in the TAEF logs. That means when you just go to the
+# "Checks" page on GitHub, the Azure DevOps integration doesn't have anything
+# meaningful to say other than "PowerShell exited with code '1'". If we however,
+# just manually emit the test names formatted with "#[error]" in front of them,
+# well, then the integration will all work like magic.
+
+# Load the test results as a XML object
+$testResults = [xml](Get-Content -Path $XUnitOutputPath)
+
+# Our XML looks like:
+# <assemblies>
+#   <assembly name="MUXControls.Test.dll" test-framework="TAEF" run-date="2023-08-14" run-time="11:38:01" total="524" passed="520" failed="4" skipped="1" time="8943" errors="0">
+#     <collection total="524" passed="520" failed="4" skipped="1" name="Test collection" time="8943">
+#       <test name="ControlCoreTests::TestSimpleClickSelection" type="ControlCoreTests" method="TestSimpleClickSelection" time="0.016" result="Fail">
+
+# Iterate over all the assemblies and print all the tests that failed
+foreach ($assembly in $testResults.assemblies.assembly) {
+    foreach ($collection in $assembly.collection) {
+        foreach ($test in $collection.test) {
+            if ($test.result -eq "Fail") {
+                # This particular format is taken from the Azure DevOps documentation:
+                # https://github.com/microsoft/azure-pipelines-tasks/blob/master/docs/authoring/commands.md
+                # This will treat this line as an error message
+                Write-Output "##vso[task.logissue type=error]$($test.name) Failed"
+            }
+        }
+    }
+}


Test patch:
diff --git a/build/pipelines/templates-v2/job-test-project.yml b/build/pipelines/templates-v2/job-test-project.yml
index 483a32893ce..d8688e36aaf 100644
--- a/build/pipelines/templates-v2/job-test-project.yml
+++ b/build/pipelines/templates-v2/job-test-project.yml
@@ -65,6 +65,14 @@ jobs:
       arguments: -WttInputPath '${{ parameters.testLogPath }}' -WttSingleRerunInputPath 'unused.wtl' -WttMultipleRerunInputPath 'unused2.wtl' -XUnitOutputPath 'onBuildMachineResults.xml' -TestNamePrefix '$(BuildConfiguration).$(BuildPlatform)'
     condition: ne(variables['PGOBuildMode'], 'Instrument')
 
+  - task: PowerShell@2
+    displayName: 'Manually log test failures'
+    inputs:
+      targetType: filePath
+      filePath: build\Helix\OutputTestErrorsForAzureDevops.ps1
+      arguments: -XUnitOutputPath 'onBuildMachineResults.xml'
+    condition: ne(variables['PGOBuildMode'], 'Instrument')
+
   - task: PublishTestResults@2
     displayName: 'Upload converted test logs'
     condition: ne(variables['PGOBuildMode'], 'Instrument')