Added more fail paths for data serialization. (#1084)

IDataSource tests don't support duplicate test names, or unserializable objects. Added bail out paths and warnings for those scnerios.

src/Adapter/MSTest.CoreAdapter/Discovery/AssemblyEnumerator.cs

@@ -357,13 +357,27 @@ private bool ProcessTestDataSourceTests(UnitTestElement test, MethodInfo methodI
             foreach (var dataSource in testDataSources)
             {
                 var data = dataSource.GetData(methodInfo);
-                var discoveredTests = new List<UnitTestElement>();
+                var discoveredTests = new Dictionary<string, UnitTestElement>();
+                var discoveredIndex = new Dictionary<string, int>();
                 var serializationFailed = false;
+                var index = 0;
 
                 foreach (var d in data)
                 {
                     var discoveredTest = test.Clone();
-                    discoveredTest.DisplayName = dataSource.GetDisplayName(methodInfo, d);
+                    discoveredTest.DisplayName = dataSource.GetDisplayName(methodInfo, d) ?? discoveredTest.DisplayName;
+
+                    // if we have a duplicate test name don't expand the test, bail out.
+                    if (discoveredTests.ContainsKey(discoveredTest.DisplayName))
+                    {
+                        var firstSeen = discoveredIndex[discoveredTest.DisplayName];
+                        var warning = string.Format(CultureInfo.CurrentCulture, Resource.CannotExpandIDataSourceAttribute_DuplicateDisplayName, firstSeen, index, discoveredTest.DisplayName);
+                        warning = string.Format(CultureInfo.CurrentUICulture, Resource.CannotExpandIDataSourceAttribute, test.TestMethod.ManagedTypeName, test.TestMethod.ManagedMethodName, warning);
+                        PlatformServiceProvider.Instance.AdapterTraceLogger.LogWarning($"DynamicDataEnumarator: {warning}");
+
+                        serializationFailed = true;
+                        break;
+                    }
 
                     try
                     {
@@ -372,22 +386,27 @@ private bool ProcessTestDataSourceTests(UnitTestElement test, MethodInfo methodI
                     }
                     catch (SerializationException)
                     {
+                        var firstSeen = discoveredIndex[discoveredTest.DisplayName];
+                        var warning = string.Format(CultureInfo.CurrentCulture, Resource.CannotExpandIDataSourceAttribute_CannotSerialize, index, discoveredTest.DisplayName);
+                        warning = string.Format(CultureInfo.CurrentUICulture, Resource.CannotExpandIDataSourceAttribute, test.TestMethod.ManagedTypeName, test.TestMethod.ManagedMethodName, warning);
+                        PlatformServiceProvider.Instance.AdapterTraceLogger.LogWarning($"DynamicDataEnumarator: {warning}");
+
                         serializationFailed = true;
                         break;
                     }
 
-                    discoveredTests.Add(discoveredTest);
+                    discoveredTests[discoveredTest.DisplayName] = discoveredTest;
+                    discoveredIndex[discoveredTest.DisplayName] = index++;
                 }
 
                 // Serialization failed for the type, bail out.
                 if (serializationFailed)
                 {
                     tests.Add(test);
-
                     break;
                 }
 
-                tests.AddRange(discoveredTests);
+                tests.AddRange(discoveredTests.Values);
             }
 
             return true;

src/Adapter/MSTest.CoreAdapter/MSTest.CoreAdapter.csproj

@@ -1,7 +1,6 @@
 <?xml version="1.0" encoding="utf-8"?>
 <Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
   <Import Project="$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props" Condition="Exists('$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props')" />
-
   <PropertyGroup>
     <ProjectGuid>{98BA6D2C-1F3D-4636-8E1D-D4932B7A253D}</ProjectGuid>
     <OutputType>Library</OutputType>
@@ -27,12 +26,10 @@
     <ErrorReport>prompt</ErrorReport>
     <WarningLevel>4</WarningLevel>
   </PropertyGroup>
-
   <ItemGroup>
     <Reference Include="System.Collections.Concurrent">
       <HintPath>C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework\.NETPortable\v4.5\System.Collections.Concurrent.dll</HintPath>
     </Reference>
-
     <ProjectReference Include="$(RepoRoot)src\Adapter\PlatformServices.Interface\PlatformServices.Interface.csproj">
       <Project>{bbc99a6b-4490-49dd-9c12-af2c1e95576e}</Project>
       <Name>PlatformServices.Interface</Name>
@@ -54,19 +51,16 @@
       <Private>False</Private>
     </ProjectReference>
   </ItemGroup>
-
   <ItemGroup>
     <PackageReference Include="Microsoft.TestPlatform.AdapterUtilities" Version="$(MicrosoftNETTestSdkVersion)" />
     <PackageReference Include="Microsoft.TestPlatform.ObjectModel" Version="$(MicrosoftNETTestSdkVersion)" />
-
     <PackageReference Include="MicroBuild.Core" Version="$(MicroBuildCoreVersion)" PrivateAssets="all">
       <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
     </PackageReference>
     <PackageReference Include="StyleCop.Analyzers" Version="$(StyleCopAnalyzersVersion)" PrivateAssets="all">
       <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
     </PackageReference>
   </ItemGroup>
-
   <ItemGroup>
     <Compile Include="Discovery\AssemblyEnumerator.cs" />
     <Compile Include="Discovery\AssemblyEnumeratorWrapper.cs" />
@@ -127,32 +121,23 @@
       <DependentUpon>Resource.resx</DependentUpon>
     </Compile>
     <Compile Include="RunConfigurationSettings.cs" />
-
     <None Include="app.config" />
   </ItemGroup>
   <ItemGroup>
     <EmbeddedResource Include="Resources\Resource.resx">
       <Generator>ResXFileCodeGenerator</Generator>
       <LastGenOutput>Resource.Designer.cs</LastGenOutput>
       <CustomToolNamespace>Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter</CustomToolNamespace>
+      <SubType>Designer</SubType>
     </EmbeddedResource>
   </ItemGroup>
-
   <Import Project="$(MSBuildExtensionsPath32)\Microsoft\Portable\$(TargetFrameworkVersion)\Microsoft.Portable.CSharp.targets" />
-
   <Target Name="CopyMSBuildScriptsFiles" DependsOnTargets="CoreCompile" AfterTargets="CoreCompile">
     <ItemGroup>
       <FileToCopy Include="$(ProjectDir)..\Build\**\*.*" />
     </ItemGroup>
-
-    <Copy SourceFiles="@(FileToCopy)" 
-          DestinationFiles="@(FileToCopy->'$(OutDir)..\Build\%(RecursiveDir)%(Filename)%(Extension)')"
-          SkipUnchangedFiles="True" OverwriteReadOnlyFiles="True" Retries="3" RetryDelayMilliseconds="500"
-          UseHardlinksIfPossible="False" UseSymboliclinksIfPossible="False" ErrorIfLinkFails="False"
-
-          Condition="@(FileToCopy->Count()) > 0">
-
-      <Output TaskParameter="DestinationFiles" ItemName="FileWrites"/>
+    <Copy SourceFiles="@(FileToCopy)" DestinationFiles="@(FileToCopy->'$(OutDir)..\Build\%(RecursiveDir)%(Filename)%(Extension)')" SkipUnchangedFiles="True" OverwriteReadOnlyFiles="True" Retries="3" RetryDelayMilliseconds="500" UseHardlinksIfPossible="False" UseSymboliclinksIfPossible="False" ErrorIfLinkFails="False" Condition="@(FileToCopy-&gt;Count()) &gt; 0">
+      <Output TaskParameter="DestinationFiles" ItemName="FileWrites" />
     </Copy>
   </Target>
 </Project>

src/Adapter/MSTest.CoreAdapter/Resources/Resource.resx

@@ -368,4 +368,20 @@ Error: {1}</value>
   <data name="OlderTFMVersionFoundClassCleanup" xml:space="preserve">
     <value>An older version of MSTestV2 package is loaded in assembly, test cleanup methods might not run as expected. Please make sure all your test projects references MSTest packages newer then version 2.2.8.</value>
   </data>
+  <data name="CannotExpandIDataSourceAttribute" xml:space="preserve">
+    <value>Exception occurred while expanding IDataSource rows from attribute on "{0}.{1}": {2}</value>
+    <comment>{0}: TypeName with namespace, 
+{1}: Method name,
+{2}: CannotExpandIDataSourceAttribute_DuplicateDisplayName or CannotExpandIDataSourceAttribute_CannotSerialize</comment>
+  </data>
+  <data name="CannotExpandIDataSourceAttribute_DuplicateDisplayName" xml:space="preserve">
+    <value>Display name "{2}" on indexes {0} and {1} are duplicate. Display names should be unique.</value>
+    <comment>{0}, {1}: Zero based index if an element inside of an array
+{2}: Test display name.</comment>
+  </data>
+  <data name="CannotExpandIDataSourceAttribute_CannotSerialize" xml:space="preserve">
+    <value>Data on index {0} for "{1}" cannot be serialized. All data provided through "IDataSource" should be serializable. If you need to test non-serializable data sources, please make sure you add "TestDataSourceDiscovery" attribute on your test assembly and set the discovery option to "DuringExecution".</value>
+    <comment>{0}: Zero based index if an element inside of an array,
+{1}: Test name</comment>
+  </data>
 </root>

test/E2ETests/DiscoveryAndExecutionTests/TestDataSourceExtensibilityTests.cs

@@ -104,5 +104,20 @@ public void ExecuteCustomTestExtensibilityWithTestDataTests()
                 "CustomTestMethod2 (C)"
             );
         }
+
+        [TestMethod]
+        public void BailOutWhenDuplicateTestDisplayName() 
+        {
+            // Arrange
+            var assemblyPath = Path.IsPathRooted(TestAssembly) ? TestAssembly : this.GetAssetFullPath(TestAssembly);
+
+            // Act
+            var testCases = DiscoverTests(assemblyPath, "Name~DynamicDataDiscoveryBailOutTestMethod1");
+            var testResults = RunTests(assemblyPath, testCases);
+
+            // Assert
+            Assert.That.TestsDiscovered(testCases, "FxExtensibilityTestProject.DynamicDataDiscoveryBailOutTests.DynamicDataDiscoveryBailOutTestMethod1");
+            Assert.That.PassedTestCount(testResults, 3);
+        }
     }
 }

test/E2ETests/DiscoveryAndExecutionTests/Utilities/CLITestBase.discovery.cs

@@ -44,9 +44,6 @@ internal ReadOnlyCollection<TestResult> RunTests(string source, IEnumerable<Test
             return frameworkHandle.GetFlattenedTestResults().ToList().AsReadOnly();
         }
 
-
-
-
         #region Helper classes
         private MSTestSettings GetSettings(bool captureDebugTraceValue)
         {

test/E2ETests/TestAssets/FxExtensibilityTestProject/CustomTestExTests.cs

@@ -1,4 +1,7 @@
-namespace FxExtensibilityTestProject
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+namespace FxExtensibilityTestProject
 {
     using System.Collections.Generic;
     using Microsoft.VisualStudio.TestTools.UnitTesting;

test/E2ETests/TestAssets/FxExtensibilityTestProject/DynamicDataDiscoveryBailOutTests.cs

@@ -0,0 +1,26 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+namespace FxExtensibilityTestProject
+{
+    using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+    using System.Collections.Generic;
+
+    [TestClass]
+    public class DynamicDataDiscoveryBailOutTests
+    {
+        public static IEnumerable<object[]> DuplicateDisplayName()
+        {
+            yield return new object[] { null };
+            yield return new object[] { new List<int>() };
+            yield return new object[] { new List<int>() { 0, 1, 2 } };
+        }
+
+        [TestMethod]
+        [DynamicData(nameof(DuplicateDisplayName), DynamicDataSourceType.Method)]
+        public void DynamicDataDiscoveryBailOutTestMethod1(IEnumerable<int> items)
+        {
+        }
+    }
+}

test/E2ETests/TestAssets/FxExtensibilityTestProject/FxExtensibilityTestProject.csproj

@@ -1,7 +1,6 @@
 <?xml version="1.0" encoding="utf-8"?>
 <Project ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
   <Import Project="$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props" Condition="Exists('$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props')" />
-
   <PropertyGroup>
     <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
     <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
@@ -35,10 +34,8 @@
     <ErrorReport>prompt</ErrorReport>
     <WarningLevel>4</WarningLevel>
   </PropertyGroup>
-
   <ItemGroup>
     <Reference Include="System" />
-
     <ProjectReference Include="$(RepoRoot)samples\FxExtensibility\FxExtensibility.csproj">
       <Project>{a82770c0-1ff5-43c7-8790-471d5e4f8d6e}</Project>
       <Name>FxExtensibility</Name>
@@ -48,16 +45,15 @@
       <Name>MSTest.Core</Name>
     </ProjectReference>
   </ItemGroup>
-
   <ItemGroup>
     <Compile Include="AssertExTest.cs" />
+    <Compile Include="DynamicDataDiscoveryBailOutTests.cs" />
     <Compile Include="Properties\AssemblyInfo.cs" />
     <Compile Include="DynamicDataExTests.cs" />
     <Compile Include="DynamicDataExMoreTests.cs" />
     <Compile Include="CustomTestExTests.cs" />
     <Compile Include="TestDataSourceExTests.cs" />
   </ItemGroup>
-
   <Import Project="$(VSToolsPath)\TeamTest\Microsoft.TestTools.targets" Condition="Exists('$(VSToolsPath)\TeamTest\Microsoft.TestTools.targets')" />
   <Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
 </Project>

test/E2ETests/TestAssets/FxExtensibilityTestProject/TestDataSourceExTests.cs

@@ -1,8 +1,11 @@
-using System;
-using Microsoft.VisualStudio.TestTools.UnitTesting;
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 namespace FxExtensibilityTestProject
 {
+    using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+    using System;
     using System.Collections.Generic;
     using System.Globalization;
     using System.Reflection;