Aborting test run when source and target frameworks/architectures are incompatible #1789

vagisha-nidhi · 2018-10-09T06:07:13Z

Description

Discrepancy in test state with actual/target framework/platform settings

Current Scenario :
Dll given : NETFramework 4.5
Target framework specified by the user : NetCoreApp
The vstest fails with unactionable message on what went wrong :

After change :

If two dlls of different frameworks/architectures are provided
Example : NetCoreApp 2.0 and NetFramework 4.6.1, the error message before change was

In case of platform, when both x64 & x86 DLL is passed x64 dll is ignored, therefore we are throwing exception in this case as well.
After change :

Same messages are displayed with architecture incompatibility.

Related issue

#1729

vagisha-nidhi · 2018-10-22T08:38:15Z

@dotnet-bot Test this again.

mmitche · 2018-10-24T14:17:08Z

@dotnet-bot test this please

singhsarab · 2018-10-25T09:26:29Z

src/vstest.console/Resources/Resources.resx

@@ -718,4 +718,7 @@
  <data name="InvalidLoggerArgument" xml:space="preserve">
    <value>Logger argument '{0}' is not valid.</value>
  </data>
+  <data name="ConflictInFrameworkPlatform" xml:space="preserve">
+    <value>Conflicts in framework/platform identifier of provided sources.</value>


Please change this to a more actionable message.
Suggestion:
"The given test sources target multiple frameworks/platforms that are incompatible. Please make sure the sources target the same framework and platform"

@PBoraMSFT @pvlakshm Can you please approve.

@PBoraMSFT @pvlakshm Is this message "The given test sources target multiple frameworks/platforms that are incompatible. Please make sure the sources target the same framework and platform" okay?

…getingDifferentFrameworks

PBoraMSFT · 2018-10-29T10:22:34Z

Test run was aborted as the specified framework / platform {fx-value/plat-value} is incompatible with the target framework / platform of the test assemblies {fx-value/plat-value}. Ensure that the target framework and platform of the test assemblies match the specified value.

singhsarab · 2018-10-25T09:29:22Z

src/vstest.console/TestPlatformHelpers/TestRequestManager.cs

-                    // Update frmaework and platform if required. For commandline scenario update happens in ArgumentProcessor.
+                    if (isFrameworkIncompatible || isPlatformIncompatible)
+                    {
+                        throw new TestPlatformException(Resources.ConflictInFrameworkPlatform);


Should we have separate messages? It just makes it clear which exactly caused the failure #Resolved

Updated the message in the description #Resolved

singhsarab · 2018-11-16T07:01:58Z

src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

@@ -612,18 +612,21 @@ public static bool TryGetFrameworkXml(XPathNavigator runSettingsNavigator, out s
        /// Returns the sources matching the specified platform and framework settings.
        /// For incompatible sources, warning is added to incompatibleSettingWarning.
        /// </summary>
-        public static IEnumerable<String> FilterCompatibleSources(Architecture chosenPlatform, Framework chosenFramework, IDictionary<String, Architecture> sourcePlatforms, IDictionary<String, Framework> sourceFrameworks, out String incompatibleSettingWarning)
+        public static IEnumerable<String> FilterCompatibleSources(Architecture chosenPlatform, Framework chosenFramework, IDictionary<String, Architecture> sourcePlatforms, IDictionary<String, Framework> sourceFrameworks, out String incompatibleSettingWarning, out bool incompatibilityErrorFound)
        {


Can we avoid multiple outs ?
It just makes it unreadable and confusing.

Ideally, we should change this method to TryFilterCompatibleSources or Throw an exception and catch it in the caller. #Resolved

One way is to leave this method as it is and using public static bool TryGetSettingIncompatibility(Architecture chosenPlatform, Framework chosenFramework, IDictionary<String, Architecture> sourcePlatforms, IDictionary<String, Framework> sourceFrameworks) to return if there is any incompatility found. #Resolved

Split it into two functions as suggested TryGetSettingIncompatibility(Architecture chosenPlatform, Framework chosenFramework, IDictionary<String, Architecture> sourcePlatforms, IDictionary<String, Framework> sourceFrameworks) #Resolved

singhsarab · 2018-11-16T07:03:32Z

src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

+        /// </summary>
+        private static bool IsPlatformArchitectureIncompatible(Architecture sourcePlatform, Architecture targetPlatform)
+        {
+            if (sourcePlatform == Architecture.X86 && targetPlatform == Architecture.X64 ||


Can we check not equals for source and target platform? #Resolved

There are different platforms(X86, X64, ARM, AnyCPU)
If we simply check for not equals, any of the incompatibility will be marked as true.
This might cause unnecessary incompatibility. For example source = Architecture.X64 is okay to run with target = Architecture.AnyCPU.
Do we want to change that behavior? #Resolved

singhsarab · 2018-11-16T07:06:14Z

src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

+            string sourceFrameworkName = sourceFramework.Name.Split(',')[0];
+            string targetFrameworkName = targetFramework.Name.Split(',')[0];
+
+            if(sourceFrameworkName.Equals(".NETFramework",StringComparison.OrdinalIgnoreCase) && targetFrameworkName.Equals(".NETCoreApp", StringComparison.OrdinalIgnoreCase) ||


What happens when we have NetStandard ? #Resolved

NetStandard will have no incompatibility with any of the other frameworks(NETFrameworks and NETCore).
Should it fail with NETStandard as well? #Resolved

singhsarab · 2018-11-16T07:08:46Z

src/vstest.console/TestPlatformHelpers/TestRequestManager.cs

@@ -363,12 +363,17 @@ private bool UpdateRunSettingsIfRequired(string runsettingsXml, List<string> sou

                    var navigator = document.CreateNavigator();

-                    var inferedFramework = inferHelper.AutoDetectFramework(sources, sourceFrameworks);
+                    var inferedFramework = inferHelper.AutoDetectFramework(sources, sourceFrameworks, out var isFrameworkIncompatible);


TryGetCompatibleFramework, and return true/false based on the detection #Resolved

Nit: Inferred #Resolved

How should be the method signature?
Current : public Framework AutoDetectFramework(List<string> sources, IDictionary<string, Framework> sourceFrameworkVersions, out bool isFrameworkIncompatible)
Possible public bool TryGetCompatibleFramework(List<string> sources, IDictionary<string, Framework> sourceFrameworkVersions, out Framework inferredFramework)
Shall I make this change? This was done to use the previous method signature as it is. #Resolved

singhsarab · 2018-11-16T08:51:06Z

test/vstest.console.UnitTests/TestPlatformHelpers/TestRequestManagerTests.cs

@@ -767,10 +768,82 @@ public void DiscoverTestsShouldNotUpdateFrameworkAndPlatformInCommandLineScenari
            this.mockAssemblyMetadataProvider.Verify(a => a.GetArchitecture(It.IsAny<string>()), Times.Once);
            this.mockAssemblyMetadataProvider.Verify(a => a.GetFrameWork(It.IsAny<string>()), Times.Once);

-            Assert.IsFalse(actualDiscoveryCriteria.RunSettings.Contains(Constants.DotNetFramework46));
+            Assert.IsFalse(actualDiscoveryCriteria.RunSettings.Contains("Framework"));


Can we make this check more strong ?

…getingDifferentFrameworks

singhsarab · 2018-12-22T05:54:04Z

src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

@@ -649,9 +649,25 @@ public static IEnumerable<String> FilterCompatibleSources(Architecture chosenPla
        }

        /// <summary>
-        /// Returns true if source settings are incomaptible with target settings.
+        /// Returns true if framework runtime of target framework is incompatible with any source framework


What does "framework runtime of target framework" imply ?

The term runtime is removed. Basically wanted to distinguish if tw frameworks are different in terms that one is framework and the other is core (version not making any difference)

singhsarab · 2018-12-22T05:56:29Z

src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

        /// </summary>
-        private static bool IsSettingIncompatible(Architecture sourcePlatform,
+        public static bool TryGetSettingIncompatibility(Architecture chosenPlatform, Framework chosenFramework, IDictionary<String, Architecture> sourcePlatforms, IDictionary<String, Framework> sourceFrameworks)


The method signature, TryGet* implies it has an out, out of the Incompatibility.
If we are simply returning the bool, why not IsSettingIncompatible?

Done #Resolved

singhsarab · 2018-12-22T05:57:35Z

src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

@@ -685,5 +701,21 @@ private static bool IsFrameworkIncompatible(Framework sourceFramework, Framework
            }
            return !sourceFramework.Name.Equals(targetFramework.Name, StringComparison.OrdinalIgnoreCase);
        }
+
+        /// <summary>
+        /// Returns true if source Framework Runtime is incompatible with target Framework.


Framework and runtime are two different concepts.
Let's just call it Framework.

singhsarab · 2018-12-22T06:34:09Z

src/Microsoft.TestPlatform.ObjectModel/Resources/CommonResources.resx

@@ -121,7 +121,7 @@
    <value>The parameter cannot be null or empty.</value>
  </data>
  <data name="DisplayChosenSettings" xml:space="preserve">
-    <value>Test run will use DLL(s) built for framework {0} and platform {1}. Following DLL(s) do not match framework/platform settings.{2}Go to {3} for more details on managing these settings.
+    <value>The specified framework {0} and platform {1} is incompatible with the target framework/platform of test assemblies. Following DLL(s) do not match framework/platform settings.{2}Ensure that the target framework and platform of the test assemblies match the specified value. Go to {3} for more details on managing these settings.


There are some redundancies and message looks very big.
How about. "The following DLL(s) do not match the specified framework {0} and platform {1} settings.{2}Ensure sure the test assemblies target the specified framework/platform settings. Go to {3} for more details on managing these settings."

/cc: @PBoraMSFT #Resolved

Message updated #Resolved

…getingDifferentFrameworks

vagisha-nidhi · 2019-01-09T08:16:01Z

@mayankbansal018 Can you please review?

mayankbansal018 · 2019-01-14T06:29:13Z

src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

+                string sourceFrameworkName = actualFramework.Name.Split(',')[0];
+                string targetFrameworkName = chosenFramework.Name.Split(',')[0];
+
+                if (sourceFrameworkName.Equals(".NETFramework", StringComparison.OrdinalIgnoreCase) && targetFrameworkName.Equals(".NETCoreApp", StringComparison.OrdinalIgnoreCase) ||


NETFramework [](start = 49, length = 12)

Make these strings as constants

mayankbansal018 · 2019-01-14T07:09:13Z

src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

        {
-            return IsPlatformIncompatible(sourcePlatform, targetPlatform) || IsFrameworkIncompatible(sourceFramework, targetFramework);
+            var res =  IsPlatformIncompatible(sourcePlatform, targetPlatform) || IsFrameworkIncompatible(sourceFramework, targetFramework);


nit: no change needed #Resolved

Restructured this code. IsSettingIncompatible is directly inferring platform/frameworks incompatibility without calling anything else #Resolved

mayankbansal018 · 2019-01-14T07:10:46Z

src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

-        /// Returns true if source settings are incomaptible with target settings.
+        /// Returns true if target framework is incompatible with any source framework, that is run needs to be aborted
+        /// </summary>
+        public static bool IsFrameworkIncompatible(Framework chosenFramework, IDictionary<String, Framework> sourceFrameworks)


public static bool IsFrameworkInc [](start = 8, length = 33)

nit: please move above

mayankbansal018 · 2019-01-14T07:12:51Z

src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

-        /// Returns true if source settings are incomaptible with target settings.
+        /// Returns true if target framework is incompatible with any source framework, that is run needs to be aborted
+        /// </summary>
+        public static bool IsFrameworkIncompatible(Framework chosenFramework, IDictionary<String, Framework> sourceFrameworks)


public static bool IsFrameworkIncompatibl [](start = 8, length = 41)

do we actually need this.

We need this to determine if the frameworks(full vs core scenario is happening), hence needing to be aborted. Restructured the other IsFrameworkIncompatible

mayankbansal018 · 2019-01-14T07:13:51Z

src/vstest.console/CommandLine/InferHelper.cs

        /// </summary>
-        public Architecture AutoDetectArchitecture(List<string> sources, IDictionary<string, Architecture> sourcePlatforms)
+        public bool TryGetCompatibleArchitecture(List<string> sources, IDictionary<string, Architecture> sourcePlatforms, out Architecture inferredArchitecture)


This functions is still auto detecting Fx, please rename it accordingly. #Resolved

Renamed to TryGetAutoDetectCompatibleArcitecture and TryGetAutoDetectCompatibleFramework #Resolved

…isha-nidhi/vstest into TargetingDifferentFrameworks

mayankbansal018 · 2019-01-15T12:22:43Z

@dotnet-bot test TestPlatform.PR please

vagisha-nidhi · 2019-01-16T04:11:48Z

@dotnet-bot Test this please

…isha-nidhi/vstest into TargetingDifferentFrameworks

mayankbansal018 · 2019-02-12T06:18:28Z

src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

+                    isFrameworkInCompatible = isFrameworkInCompatible || !actualFramework.Name.Equals(targetFramework.Name, StringComparison.OrdinalIgnoreCase);
+                }
+
+                string sourceFrameworkName = actualFramework.Name.Split(',')[0];


If we are checking with version, then we don't need to follow up on w/o version comparison

mayankbansal018 · 2019-02-12T06:20:03Z

src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

+                string sourceFrameworkName = actualFramework.Name.Split(',')[0];
+                string targetFrameworkName = targetFramework.Name.Split(',')[0];
+
+                if (sourceFrameworkName.Equals(DotNetFrameworkString, StringComparison.OrdinalIgnoreCase) && targetFrameworkName.Equals(DotNetCoreString, StringComparison.OrdinalIgnoreCase) ||


DotNetFrameworkString [](start = 47, length = 21)

Lets keep the framework comparison logic same as above, we shouldn't have to compare them with Const strings, & mark them as incompatible.

…getingDifferentFrameworks

…isha-nidhi/vstest into TargetingDifferentFrameworks

samneefs · 2019-03-01T15:12:05Z

This error ("The given test sources target multiple frameworks/platforms that are incompatible. Please make sure the sources target the same framework and platform.") now throws when running tests where the the tested method is in a .NET Framework project which references a .NET Standard project (not the other way around).
If I'm not mistaken, this shouldn't throw an error and is allowed behavior (.NET Framework can access .NET Standard).
If I'm not mistaken the error should only throw when the .NET Standard project references .NET Framework code (not sure about the specifics), which isn't the case here.

This occurs in on Azure DevOps, with latest version (=16.0.0) of "Visual Studio Test Platform Installer"
When downgrading the test framework to 15.9.0 (latest version before this change), then the tests run without a problem.

…ures are incompatible (microsoft#1789)" This reverts commit 37678c4.

* Revert "LOC CHECKIN | Microsoft/vstest master | 20190213 (#1923)" This reverts commit 08b8fab. * Revert "Aborting test run when source and target frameworks/architectures are incompatible (#1789)" This reverts commit 37678c4.

vagisha-nidhi added 2 commits October 23, 2018 16:22

ExceptionWhenTargetingDiffFramworksPlat

f60b214

Acceptance tests updated

0a8e968

vagisha-nidhi force-pushed the TargetingDifferentFrameworks branch from fe61db5 to 0a8e968 Compare October 23, 2018 10:54

vagisha-nidhi requested review from singhsarab and mayankbansal018 October 25, 2018 05:33

singhsarab reviewed Oct 25, 2018

View reviewed changes

Merge branch 'master' of https://github.com/Microsoft/vstest into Tar…

ce5de2f

…getingDifferentFrameworks

Message change in conflict dlls

37fa219

mayankbansal018 closed this Nov 14, 2018

mayankbansal018 reopened this Nov 14, 2018

singhsarab suggested changes Nov 16, 2018

View reviewed changes

singhsarab reviewed Nov 16, 2018

View reviewed changes

vagisha-nidhi added 2 commits December 1, 2018 14:46

Changes in methods

75e2164

Merge branch 'master' of https://github.com/Microsoft/vstest into Tar…

92fe52c

…getingDifferentFrameworks

vagisha-nidhi force-pushed the TargetingDifferentFrameworks branch from 9bf652d to 92fe52c Compare December 1, 2018 09:17

vagisha-nidhi added 3 commits December 1, 2018 15:54

minor fix in test

9943636

fix in test

30ebd14

improved error message

e6230ba

singhsarab reviewed Dec 22, 2018

View reviewed changes

vagisha-nidhi added 3 commits December 31, 2018 12:39

PR comments

3fa02c1

fix acceptance test

ce59bb3

Merge branch 'master' of https://github.com/Microsoft/vstest into Tar…

d7057e2

…getingDifferentFrameworks

vagisha-nidhi force-pushed the TargetingDifferentFrameworks branch from b52f4b3 to d7057e2 Compare January 2, 2019 10:36

mayankbansal018 reviewed Jan 14, 2019

View reviewed changes

vagisha-nidhi and others added 5 commits January 14, 2019 14:14

Refactoring InferRunSettingsHelper

220c955

Refactor

c9740ea

Merge branch 'master' into TargetingDifferentFrameworks

e4f233f

Tests updated

2e66caa

Merge branch 'TargetingDifferentFrameworks' of https://github.com/vag…

550b4cc

…isha-nidhi/vstest into TargetingDifferentFrameworks

vagisha-nidhi and others added 8 commits January 16, 2019 09:45

Update TestRequestManager.cs

10bc3f3

Restructured and Added test

ed39fbd

Merge branch 'TargetingDifferentFrameworks' of https://github.com/vag…

58b3dfa

…isha-nidhi/vstest into TargetingDifferentFrameworks

Update TestRequestManager.cs

bc8c120

minor change

e850846

Merge branch 'TargetingDifferentFrameworks' of https://github.com/vag…

b033b87

…isha-nidhi/vstest into TargetingDifferentFrameworks

minor change

3d5b844

Merge branch 'master' into TargetingDifferentFrameworks

615fbac

mayankbansal018 reviewed Feb 12, 2019

View reviewed changes

vagisha-nidhi and others added 4 commits February 12, 2019 12:20

Merge branch 'master' of https://github.com/Microsoft/vstest into Tar…

35ba9e5

…getingDifferentFrameworks

Merge branch 'master' into TargetingDifferentFrameworks

1ddc150

Modified logic in IsFrameworkIncompatible

6bcc76c

Merge branch 'TargetingDifferentFrameworks' of https://github.com/vag…

7836605

…isha-nidhi/vstest into TargetingDifferentFrameworks

mayankbansal018 approved these changes Feb 13, 2019

View reviewed changes

Removing unused variable

53b609b

singhsarab approved these changes Feb 13, 2019

View reviewed changes

vagisha-nidhi merged commit 37678c4 into microsoft:master Feb 13, 2019

vagisha-nidhi added a commit to vagisha-nidhi/vstest that referenced this pull request Mar 4, 2019

Revert "Aborting test run when source and target frameworks/architect…

be347ef

…ures are incompatible (microsoft#1789)" This reverts commit 37678c4.

Aborting test run when source and target frameworks/architectures are incompatible #1789

Aborting test run when source and target frameworks/architectures are incompatible #1789

Conversation

vagisha-nidhi commented Oct 9, 2018 • edited Loading

Description

Related issue

vagisha-nidhi commented Oct 22, 2018

mmitche commented Oct 24, 2018

singhsarab Oct 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PBoraMSFT commented Oct 29, 2018

singhsarab Oct 25, 2018 • edited by vagisha-nidhi Loading

Choose a reason for hiding this comment

vagisha-nidhi Dec 1, 2018 • edited Loading

Choose a reason for hiding this comment

singhsarab Nov 16, 2018 • edited by vagisha-nidhi Loading

Choose a reason for hiding this comment

vagisha-nidhi Nov 30, 2018 • edited Loading

Choose a reason for hiding this comment

vagisha-nidhi Dec 1, 2018 • edited Loading

Choose a reason for hiding this comment

singhsarab Nov 16, 2018 • edited by vagisha-nidhi Loading

Choose a reason for hiding this comment

vagisha-nidhi Nov 30, 2018 • edited Loading

Choose a reason for hiding this comment

singhsarab Nov 16, 2018 • edited by vagisha-nidhi Loading

Choose a reason for hiding this comment

vagisha-nidhi Nov 30, 2018 • edited Loading

Choose a reason for hiding this comment

singhsarab Nov 16, 2018 • edited by vagisha-nidhi Loading

Choose a reason for hiding this comment

singhsarab Nov 16, 2018 • edited by vagisha-nidhi Loading

Choose a reason for hiding this comment

vagisha-nidhi Nov 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vagisha-nidhi Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singhsarab Dec 22, 2018 • edited by vagisha-nidhi Loading

Choose a reason for hiding this comment

vagisha-nidhi Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

vagisha-nidhi commented Jan 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayankbansal018 Jan 14, 2019 • edited by vagisha-nidhi Loading

Choose a reason for hiding this comment

vagisha-nidhi Jan 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayankbansal018 Jan 14, 2019 • edited by vagisha-nidhi Loading

Choose a reason for hiding this comment

vagisha-nidhi Jan 14, 2019 • edited Loading

Choose a reason for hiding this comment

mayankbansal018 commented Jan 15, 2019

vagisha-nidhi commented Jan 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samneefs commented Mar 1, 2019

vagisha-nidhi commented Oct 9, 2018 •

edited

Loading

singhsarab Oct 25, 2018 •

edited

Loading

singhsarab Oct 25, 2018 •

edited by vagisha-nidhi

Loading

vagisha-nidhi Dec 1, 2018 •

edited

Loading

singhsarab Nov 16, 2018 •

edited by vagisha-nidhi

Loading

vagisha-nidhi Nov 30, 2018 •

edited

Loading

vagisha-nidhi Dec 1, 2018 •

edited

Loading

singhsarab Nov 16, 2018 •

edited by vagisha-nidhi

Loading

vagisha-nidhi Nov 30, 2018 •

edited

Loading

singhsarab Nov 16, 2018 •

edited by vagisha-nidhi

Loading

vagisha-nidhi Nov 30, 2018 •

edited

Loading

singhsarab Nov 16, 2018 •

edited by vagisha-nidhi

Loading

singhsarab Nov 16, 2018 •

edited by vagisha-nidhi

Loading

vagisha-nidhi Nov 30, 2018 •

edited

Loading

vagisha-nidhi Jan 3, 2019 •

edited

Loading

singhsarab Dec 22, 2018 •

edited by vagisha-nidhi

Loading

vagisha-nidhi Jan 3, 2019 •

edited

Loading

mayankbansal018 Jan 14, 2019 •

edited by vagisha-nidhi

Loading

vagisha-nidhi Jan 14, 2019 •

edited

Loading

mayankbansal018 Jan 14, 2019 •

edited by vagisha-nidhi

Loading

vagisha-nidhi Jan 14, 2019 •

edited

Loading