[16.8] Fix collect dump always (#2641)

* Fix collect dump always

* Fix acceptance tests

scripts/build.ps1

@@ -737,7 +737,7 @@ function Create-NugetPackages
 
     # Verifies that expected number of files gets shipped in nuget packages.
     # Few nuspec uses wildcard characters.
-    Verify-Nuget-Packages $packageOutputDir
+    Verify-Nuget-Packages $packageOutputDir $TPB_Version
 
     Write-Log "Create-NugetPackages: Complete. {$(Get-ElapsedTime($timer))}"
 }

scripts/build/TestPlatform.Dependencies.props

@@ -11,7 +11,7 @@
     <!-- this version also needs to be "statically" readable because the test fixture will inspect this file for the version 
     and because during the test `dotnet test` will run and re-build some of the test projects and at that time the version 
     from a build parameter would not be available, so I am writing this version from the build.ps1 script to keep it in sync --> 
-    <NETTestSdkVersion>16.8.0-dev</NETTestSdkVersion>
+    <NETTestSdkVersion>16.8.1-dev</NETTestSdkVersion>
 
     <MSTestFrameworkVersion>2.1.0</MSTestFrameworkVersion>
     <MSTestAdapterVersion>2.1.0</MSTestAdapterVersion>

scripts/build/TestPlatform.Settings.targets

@@ -5,7 +5,7 @@
     <!-- This version is read by vsts-prebuild.ps1 and is a base for the current version, this should be updated
     at the start of new iteration to the goal number. This is also used to version the local packages. This version needs to be statically
     readable when we read the file as xml, don't move it to a .props file, unless you change the build server process -->
-    <TPVersionPrefix>16.8.0</TPVersionPrefix>
+    <TPVersionPrefix>16.8.1</TPVersionPrefix>
   </PropertyGroup>
   <PropertyGroup>
     <!-- Versioning is defined from the build script. Use a default dev build if it's not defined.

scripts/verify-nupkgs.ps1

@@ -9,7 +9,7 @@ function Unzip
 }
 
 
-function Verify-Nuget-Packages($packageDirectory)
+function Verify-Nuget-Packages($packageDirectory, $version)
 {
     Write-Log "Starting Verify-Nuget-Packages."
     $expectedNumOfFiles = @{
@@ -24,7 +24,7 @@ function Verify-Nuget-Packages($packageDirectory)
                      "Microsoft.TestPlatform.TestHost" = 154;
                      "Microsoft.TestPlatform.TranslationLayer" = 121}
 
-    $nugetPackages = Get-ChildItem -Filter "*.nupkg" $packageDirectory | % { $_.FullName}
+    $nugetPackages = Get-ChildItem -Filter "*$version*.nupkg" $packageDirectory | % { $_.FullName }
 
     Write-VerboseLog "Unzip NuGet packages."
     $unzipNugetPackageDirs =  New-Object System.Collections.Generic.List[System.Object]

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs

@@ -462,7 +462,10 @@ private void SessionEndedHandler(object sender, SessionEndEventArgs args)
                 }
                 else
                 {
-                    this.logger.LogWarning(this.context.SessionDataCollectionContext, Resources.Resources.NotGeneratingSequenceFile);
+                    if (this.collectProcessDumpOnTestHostHang)
+                    {
+                        this.logger.LogWarning(this.context.SessionDataCollectionContext, Resources.Resources.NotGeneratingSequenceFile);
+                    }
                 }
 
                 if (this.uploadDumpFiles)
@@ -528,7 +531,7 @@ private void TestHostLaunchedHandler(object sender, TestHostLaunchedEventArgs ar
             try
             {
                 var dumpDirectory = this.GetDumpDirectory();
-                this.processDumpUtility.StartTriggerBasedProcessDump(args.TestHostProcessId, dumpDirectory, this.processFullDumpEnabled, this.targetFramework);
+                this.processDumpUtility.StartTriggerBasedProcessDump(args.TestHostProcessId, dumpDirectory, this.processFullDumpEnabled, this.targetFramework, this.collectDumpAlways);
             }
             catch (TestPlatformException e)
             {
@@ -585,7 +588,15 @@ private string GetTempDirectory()
         {
             if (string.IsNullOrWhiteSpace(this.tempDirectory))
             {
-                this.tempDirectory = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString());
+                // DUMP_TEMP_PATH will be used as temporary storage location
+                // for the dumps, this won't affect the dump uploads. Just the place where
+                // we store them before moving them to the final folder.
+
+                // AGENT_TEMPDIRECTORY is AzureDevops variable, which is set to path
+                // that is cleaned up after every job. This is preferable to use over
+                // just the normal temp.
+                var temp = Environment.GetEnvironmentVariable("VSTEST_DUMP_TEMP_PATH") ?? Environment.GetEnvironmentVariable("AGENT_TEMPDIRECTORY") ?? Path.GetTempPath();
+                this.tempDirectory = Path.Combine(temp, Guid.NewGuid().ToString());
                 Directory.CreateDirectory(this.tempDirectory);
                 return this.tempDirectory;
             }

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ICrashDumper.cs

@@ -5,7 +5,7 @@ namespace Microsoft.TestPlatform.Extensions.BlameDataCollector
 {
     public interface ICrashDumper
     {
-        void AttachToTargetProcess(int processId, string outputDirectory, DumpTypeOption dumpType);
+        void AttachToTargetProcess(int processId, string outputDirectory, DumpTypeOption dumpType, bool collectAlways);
 
         void WaitForDumpToFinish();
 

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Interfaces/IProcDumpArgsBuilder.cs

@@ -22,8 +22,11 @@ public interface IProcDumpArgsBuilder
         /// <param name="isFullDump">
         /// Is full dump enabled
         /// </param>
+        /// <param name="collectAlways">
+        /// Collects the dump on process exit even when there is no exception
+        /// </param>
         /// <returns>Arguments</returns>
-        string BuildTriggerBasedProcDumpArgs(int processId, string filename, IEnumerable<string> procDumpExceptionsList, bool isFullDump);
+        string BuildTriggerBasedProcDumpArgs(int processId, string filename, IEnumerable<string> procDumpExceptionsList, bool isFullDump, bool collectAlways);
 
         /// <summary>
         /// Arguments for procdump.exe for getting a dump in case of a testhost hang

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Interfaces/IProcessDumpUtility.cs

@@ -32,7 +32,10 @@ public interface IProcessDumpUtility
         /// <param name="targetFramework">
         /// The target framework of the process
         /// </param>
-        void StartTriggerBasedProcessDump(int processId, string testResultsDirectory, bool isFullDump, string targetFramework);
+        /// <param name="collectAlways">
+        /// Collect the dump on process exit even if there is no exception
+        /// </param>
+        void StartTriggerBasedProcessDump(int processId, string testResultsDirectory, bool isFullDump, string targetFramework, bool collectAlways);
 
         /// <summary>
         /// Launch proc dump process to capture dump in case of a testhost hang and wait for it to exit

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/NetClientCrashDumper.cs

@@ -5,7 +5,7 @@ namespace Microsoft.TestPlatform.Extensions.BlameDataCollector
 {
     internal class NetClientCrashDumper : ICrashDumper
     {
-        public void AttachToTargetProcess(int processId, string outputDirectory, DumpTypeOption dumpType)
+        public void AttachToTargetProcess(int processId, string outputDirectory, DumpTypeOption dumpType, bool collectAlways)
         {
             // we don't need to do anything directly here, we setup the env variables
             // in the dumper configuration, including the path

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcDumpArgsBuilder.cs

@@ -9,15 +9,15 @@ namespace Microsoft.TestPlatform.Extensions.BlameDataCollector
     public class ProcDumpArgsBuilder : IProcDumpArgsBuilder
     {
         /// <inheritdoc />
-        public string BuildTriggerBasedProcDumpArgs(int processId, string filename, IEnumerable<string> procDumpExceptionsList, bool isFullDump)
+        public string BuildTriggerBasedProcDumpArgs(int processId, string filename, IEnumerable<string> procDumpExceptionsList, bool isFullDump, bool collectAlways)
         {
             // -accepteula: Auto accept end-user license agreement
             // -e: Write a dump when the process encounters an unhandled exception. Include the 1 to create dump on first chance exceptions.
             // -g: Run as a native debugger in a managed process (no interop).
             // -t: Write a dump when the process terminates.
             // -ma: Full dump argument.
             // -f: Filter the exceptions.
-            StringBuilder procDumpArgument = new StringBuilder("-accepteula -e 1 -g -t ");
+            StringBuilder procDumpArgument = new StringBuilder($"-accepteula -e 1 -g {(collectAlways ? "-t " : string.Empty)}");
             if (isFullDump)
             {
                 procDumpArgument.Append("-ma ");

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcDumpCrashDumper.cs

@@ -69,7 +69,7 @@ public void WaitForDumpToFinish()
         }
 
         /// <inheritdoc/>
-        public void AttachToTargetProcess(int processId, string outputDirectory, DumpTypeOption dumpType)
+        public void AttachToTargetProcess(int processId, string outputDirectory, DumpTypeOption dumpType, bool collectAlways)
         {
             var process = Process.GetProcessById(processId);
             var outputFile = Path.Combine(outputDirectory, $"{process.ProcessName}_{process.Id}_{DateTime.Now:yyyyMMddTHHmmss}_crashdump.dmp");
@@ -96,7 +96,8 @@ public void AttachToTargetProcess(int processId, string outputDirectory, DumpTyp
                 processId,
                 this.dumpFileName,
                 ProcDumpExceptionsList,
-                isFullDump: dumpType == DumpTypeOption.Full);
+                isFullDump: dumpType == DumpTypeOption.Full,
+                collectAlways: collectAlways);
 
             EqtTrace.Info($"ProcDumpCrashDumper.AttachToTargetProcess: Running ProcDump with arguments: '{procDumpArgs}'.");
             this.procDumpProcess = this.processHelper.LaunchProcess(

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs

@@ -98,9 +98,9 @@ public void StartHangBasedProcessDump(int processId, string tempDirectory, bool
         }
 
         /// <inheritdoc/>
-        public void StartTriggerBasedProcessDump(int processId, string testResultsDirectory, bool isFullDump, string targetFramework)
+        public void StartTriggerBasedProcessDump(int processId, string testResultsDirectory, bool isFullDump, string targetFramework, bool collectAlways)
         {
-            this.CrashDump(processId, testResultsDirectory, isFullDump ? DumpTypeOption.Full : DumpTypeOption.Mini, targetFramework);
+            this.CrashDump(processId, testResultsDirectory, isFullDump ? DumpTypeOption.Full : DumpTypeOption.Mini, targetFramework, collectAlways);
         }
 
         /// <inheritdoc/>
@@ -109,15 +109,15 @@ public void DetachFromTargetProcess(int targetProcessId)
             this.crashDumper?.DetachFromTargetProcess(targetProcessId);
         }
 
-        private void CrashDump(int processId, string tempDirectory, DumpTypeOption dumpType, string targetFramework)
+        private void CrashDump(int processId, string tempDirectory, DumpTypeOption dumpType, string targetFramework, bool collectAlways)
         {
             var processName = this.processHelper.GetProcessName(processId);
             EqtTrace.Info($"ProcessDumpUtility.CrashDump: Creating {dumpType.ToString().ToLowerInvariant()} dump of process {processName} ({processId}) into temporary path '{tempDirectory}'.");
             this.crashDumpDirectory = tempDirectory;
 
             this.crashDumper = this.crashDumperFactory.Create(targetFramework);
             ConsoleOutput.Instance.Information(false, $"Blame: Attaching crash dump utility to process {processName} ({processId}).");
-            this.crashDumper.AttachToTargetProcess(processId, tempDirectory, dumpType);
+            this.crashDumper.AttachToTargetProcess(processId, tempDirectory, dumpType, collectAlways);
         }
 
         private void HangDump(int processId, string tempDirectory, DumpTypeOption dumpType, string targetFramework, Action<string> logWarning = null)

test/Microsoft.TestPlatform.AcceptanceTests/BlameDataCollectorTests.cs

@@ -6,6 +6,7 @@ namespace Microsoft.TestPlatform.AcceptanceTests
     using Microsoft.VisualStudio.TestTools.UnitTesting;
     using System;
     using System.IO;
+    using System.Text.RegularExpressions;
     using System.Xml;
 
     [TestClass]
@@ -41,7 +42,7 @@ public void BlameDataCollectorShouldGiveCorrectTestCaseName(RunnerInfo runnerInf
             arguments = string.Concat(arguments, $" /ResultsDirectory:{resultsDir}");
             this.InvokeVsTest(arguments);
 
-            this.VaildateOutput();
+            this.VaildateOutput("BlameUnitTestProject.UnitTest1.TestMethod2");
         }
 
         [TestMethod]
@@ -51,22 +52,59 @@ public void BlameDataCollectorShouldOutputDumpFile(RunnerInfo runnerInfo)
         {
             Environment.SetEnvironmentVariable("PROCDUMP_PATH", Path.Combine(this.testEnvironment.PackageDirectory, @"procdump\0.0.1\bin"));
 
+            AcceptanceTestBase.SetTestEnvironment(this.testEnvironment, runnerInfo);            
+            var assemblyPaths = this.BuildMultipleAssemblyPath("SimpleTestProject3.dll").Trim('\"');
+            var arguments = PrepareArguments(assemblyPaths, this.GetTestAdapterPath(), string.Empty, string.Empty, runnerInfo.InIsolationValue);
+            arguments = string.Concat(arguments, $" /Blame:CollectDump");
+            arguments = string.Concat(arguments, $" /ResultsDirectory:{resultsDir}");
+            arguments = string.Concat(arguments, " /testcasefilter:ExitWithStackoverFlow");
+            this.InvokeVsTest(arguments);
+
+            this.VaildateOutput("SampleUnitTestProject3.UnitTest1.ExitWithStackoverFlow", validateDumpFile: true);
+        }
+
+        [TestMethod]
+        [NetFullTargetFrameworkDataSource]
+        [NetCoreTargetFrameworkDataSource]
+        public void BlameDataCollectorShouldNotOutputDumpFileWhenNoCrashOccurs(RunnerInfo runnerInfo)
+        {
+            Environment.SetEnvironmentVariable("PROCDUMP_PATH", Path.Combine(this.testEnvironment.PackageDirectory, @"procdump\0.0.1\bin"));
+
             AcceptanceTestBase.SetTestEnvironment(this.testEnvironment, runnerInfo);
-            var assemblyPaths = this.GetAssetFullPath("BlameUnitTestProject.dll");
+            var assemblyPaths = this.BuildMultipleAssemblyPath("SimpleTestProject.dll").Trim('\"');
             var arguments = PrepareArguments(assemblyPaths, this.GetTestAdapterPath(), string.Empty, string.Empty, runnerInfo.InIsolationValue);
             arguments = string.Concat(arguments, $" /Blame:CollectDump");
             arguments = string.Concat(arguments, $" /ResultsDirectory:{resultsDir}");
+            arguments = string.Concat(arguments, " /testcasefilter:PassingTest");
+            this.InvokeVsTest(arguments);
+
+            StringAssert.DoesNotMatch(this.StdOut, new Regex( @"\.dmp"), "it should not collect a dump, because nothing crashed");
+        }
+
+        [TestMethod]
+        [NetFullTargetFrameworkDataSource]
+        [NetCoreTargetFrameworkDataSource]
+        public void BlameDataCollectorShouldOutputDumpFileWhenNoCrashOccursButCollectAlwaysIsEnabled(RunnerInfo runnerInfo)
+        {
+            Environment.SetEnvironmentVariable("PROCDUMP_PATH", Path.Combine(this.testEnvironment.PackageDirectory, @"procdump\0.0.1\bin"));
+
+            AcceptanceTestBase.SetTestEnvironment(this.testEnvironment, runnerInfo);
+            var assemblyPaths = this.BuildMultipleAssemblyPath("SimpleTestProject.dll").Trim('\"');
+            var arguments = PrepareArguments(assemblyPaths, this.GetTestAdapterPath(), string.Empty, string.Empty, runnerInfo.InIsolationValue);
+            arguments = string.Concat(arguments, $" /Blame:CollectDump;CollectAlways=True");
+            arguments = string.Concat(arguments, $" /ResultsDirectory:{resultsDir}");
+            arguments = string.Concat(arguments, " /testcasefilter:PassingTest");
             this.InvokeVsTest(arguments);
 
-            this.VaildateOutput(true);
+            StringAssert.Matches(this.StdOut, new Regex(@"\.dmp"), "it should collect dump, even if nothing crashed");
         }
 
-        private void VaildateOutput(bool validateDumpFile = false)
+        private void VaildateOutput(string testName, bool validateDumpFile = false)
         {
             bool isSequenceAttachmentReceived = false;
             bool isDumpAttachmentReceived = false;
             bool isValid = false;
-            this.StdErrorContains("BlameUnitTestProject.UnitTest1.TestMethod2");
+            this.StdErrorContains(testName);
             this.StdOutputContains("Sequence_");
             var resultFiles = Directory.GetFiles(this.resultsDir, "*", SearchOption.AllDirectories);
 

test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/BlameCollectorTests.cs

@@ -476,7 +476,7 @@ public void TriggerTestHostLaunchedHandlerShouldStartProcDumpUtilityIfProcDumpEn
             this.mockDataColectionEvents.Raise(x => x.TestHostLaunched += null, new TestHostLaunchedEventArgs(this.dataCollectionContext, 1234));
 
             // Verify StartProcessDumpCall
-            this.mockProcessDumpUtility.Verify(x => x.StartTriggerBasedProcessDump(1234, It.IsAny<string>(), false, It.IsAny<string>()));
+            this.mockProcessDumpUtility.Verify(x => x.StartTriggerBasedProcessDump(1234, It.IsAny<string>(), false, It.IsAny<string>(), false));
         }
 
         /// <summary>
@@ -497,7 +497,7 @@ public void TriggerTestHostLaunchedHandlerShouldStartProcDumpUtilityForFullDumpI
             this.mockDataColectionEvents.Raise(x => x.TestHostLaunched += null, new TestHostLaunchedEventArgs(this.dataCollectionContext, 1234));
 
             // Verify StartProcessDumpCall
-            this.mockProcessDumpUtility.Verify(x => x.StartTriggerBasedProcessDump(1234, It.IsAny<string>(), true, It.IsAny<string>()));
+            this.mockProcessDumpUtility.Verify(x => x.StartTriggerBasedProcessDump(1234, It.IsAny<string>(), true, It.IsAny<string>(), false));
         }
 
         /// <summary>
@@ -526,7 +526,7 @@ public void TriggerTestHostLaunchedHandlerShouldStartProcDumpUtilityForFullDumpI
             this.mockDataColectionEvents.Raise(x => x.TestHostLaunched += null, new TestHostLaunchedEventArgs(this.dataCollectionContext, 1234));
 
             // Verify StartProcessDumpCall
-            this.mockProcessDumpUtility.Verify(x => x.StartTriggerBasedProcessDump(1234, It.IsAny<string>(), true, It.IsAny<string>()));
+            this.mockProcessDumpUtility.Verify(x => x.StartTriggerBasedProcessDump(1234, It.IsAny<string>(), true, It.IsAny<string>(), false));
         }
 
         /// <summary>
@@ -646,7 +646,7 @@ public void TriggerTestHostLaunchedHandlerShouldCatchTestPlatFormExceptionsAndRe
 
             // Make StartProcessDump throw exception
             var tpex = new TestPlatformException("env var exception");
-            this.mockProcessDumpUtility.Setup(x => x.StartTriggerBasedProcessDump(1234, It.IsAny<string>(), false, It.IsAny<string>()))
+            this.mockProcessDumpUtility.Setup(x => x.StartTriggerBasedProcessDump(1234, It.IsAny<string>(), false, It.IsAny<string>(), false))
                                        .Throws(tpex);
 
             // Raise TestHostLaunched
@@ -672,7 +672,7 @@ public void TriggerTestHostLaunchedHandlerShouldCatchAllUnexpectedExceptionsAndR
 
             // Make StartProcessDump throw exception
             var ex = new Exception("start process failed");
-            this.mockProcessDumpUtility.Setup(x => x.StartTriggerBasedProcessDump(1234, It.IsAny<string>(), false, It.IsAny<string>()))
+            this.mockProcessDumpUtility.Setup(x => x.StartTriggerBasedProcessDump(1234, It.IsAny<string>(), false, It.IsAny<string>(), false))
                                        .Throws(ex);
 
             // Raise TestHostLaunched
@@ -709,9 +709,9 @@ public void CleanUp()
 
             if (collectDumpOnExit)
             {
-                var fulldumpAttribute = xmldoc.CreateAttribute(BlameDataCollector.Constants.CollectDumpAlwaysKey);
-                fulldumpAttribute.Value = "true";
-                node.Attributes.Append(fulldumpAttribute);
+                var collectDumpOnExitAttribute = xmldoc.CreateAttribute(BlameDataCollector.Constants.CollectDumpAlwaysKey);
+                collectDumpOnExitAttribute.Value = "true";
+                node.Attributes.Append(collectDumpOnExitAttribute);
             }
 
             if (colectDumpOnHang)