Adding Resolution Paths during appdomain creation (Compatibility Isuue) #404

jayaranigarg · 2018-04-19T10:24:04Z

Scenario which was broken -
If suppose TestProject has a config file having assembly binding redirect of System.Runtime to 4.1. Our TestAdapter which has a dependency of System.Runtime of 4.0, uses the above config file and tries to load System.Runtime 4.1. Since assembly resolver was not set at the time of loading of adapter, it is unable to find 4.1 version of assembly and bombs.

Fix - Adding resolution paths at the time of creation of appdomains.

Validations-

2 Projects having different adapter versions works fine with enable and Appdomain
Above scenario.

singhsarab · 2018-04-19T10:37:24Z

src/Adapter/PlatformServices.Desktop/Services/DesktopTestSourceHost.cs

@@ -75,35 +75,123 @@ internal TestSourceHost(string sourceFileName, IRunSettings runSettings, IFramew
        /// </summary>
        public void SetupHost()
        {
-            if (this.AppDomainCreationDisabledInRunSettings())
+            List<string> resolutionPaths = this.GetResolutionPaths(this.sourceFileName, VSInstallationUtilities.IsCurrentProcessRunningInPortableMode());


Please add test for ResolutionPaths being set.

smadala · 2018-04-19T10:59:24Z

src/Adapter/PlatformServices.Desktop/Services/DesktopTestSourceHost.cs


-            this.targetFrameworkVersion = this.GetTargetFrameworkVersionString(this.sourceFileName);
+                // Adding extensions folder to resolution paths


Nit: this is runner location(OM assembly location).

smadala · 2018-04-19T11:00:45Z

src/Adapter/PlatformServices.Desktop/Services/DesktopTestSourceHost.cs

+            List<string> resolutionPaths = this.GetResolutionPaths(this.sourceFileName, VSInstallationUtilities.IsCurrentProcessRunningInPortableMode());
+
+            // Check if user specified any runsettings
+            MSTestAdapterSettings adapterSettings = MSTestSettingsProvider.Settings;


Move the declaration near to usage.

smadala · 2018-04-19T11:05:36Z

src/Adapter/PlatformServices.Desktop/Services/DesktopTestSourceHost.cs

+            // Check if user specified any runsettings
+            MSTestAdapterSettings adapterSettings = MSTestSettingsProvider.Settings;
+
+            if (resolutionPaths != null && resolutionPaths.Count > 0)


This should not be happen. Even if it is empty or null, We should add below two paths.

smadala · 2018-04-19T12:20:56Z

src/Adapter/PlatformServices.Desktop/Services/DesktopTestSourceHost.cs

+                    }
+                    catch (Exception exception)
+                    {
+                        if (EqtTrace.IsErrorEnabled)


Nit: Remove if check for error scenarios.

smadala · 2018-04-19T12:21:20Z

src/Adapter/PlatformServices.Desktop/Services/DesktopTestSourceHost.cs

+                    {
+                        if (EqtTrace.IsErrorEnabled)
+                        {
+                            EqtTrace.Error(exception);


Nit: Add more details while logging the exception. At line 190 too.

smadala · 2018-04-19T12:27:33Z

src/Adapter/PlatformServices.Desktop/Services/DesktopTestSourceHost.cs

+                var configFile = this.GetConfigFileForTestSource(this.sourceFileName);
+                AppDomainUtilities.SetConfigurationFile(appDomainSetup, configFile);
+
+                this.domain = this.appDomain.CreateDomain("TestSourceHost: Enumering assembly", null, appDomainSetup);


Change the name of appdomain to sourcefilename.

smadala · 2018-04-19T12:56:05Z

src/Adapter/PlatformServices.Desktop/Services/DesktopTestSourceHost.cs

-
-            // Case when Child-appdomain was created successfully, update appbase and set assembly resolver on it
-            else if (this.domain != null)
+            if (this.domain != null)


Change this to inverted if IsAppDomainCreationDisable.

smadala · 2018-04-19T12:56:42Z

src/Adapter/PlatformServices.Desktop/Services/DesktopTestSourceHost.cs

            }

            // Log error when child-appdomain was expected to be created but wasn't created.
-            else
+            else if (!this.AppDomainCreationDisabledInRunSettings())


This is not required. Are DisableAppDomain settings from runsettings only once.

smadala · 2018-04-19T13:09:23Z

src/Adapter/PlatformServices.Desktop/Services/DesktopTestSourceHost.cs

+                    try
+                    {
+                        var additionalSearchDirectories =
+                            adapterSettings.GetDirectoryListWithRecursiveProperty(this.domain.SetupInformation.ApplicationBase);


this.domain.SetupInformation.ApplicationBase value should be testsource directory.

Reuse additionalSearchDirectories.

smadala · 2018-04-19T13:18:01Z

src/Adapter/PlatformServices.Desktop/Services/DesktopTestSourceHost.cs

+                    try
+                    {
+                        this.parentDomainAssemblyResolver = new AssemblyResolver(resolutionPaths);
+                        this.parentDomainAssemblyResolver.AddSearchDirectoriesFromRunSetting(adapterSettings.GetDirectoryListWithRecursiveProperty(null));


GetDirectoryListWithRecursiveProperty should take test source directory.

…into compatrefix

smadala · 2018-05-07T12:06:32Z

TestFx.sln

@@ -172,7 +172,9 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "DataSourceTestProject", "te
 EndProject
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "DoNotParallelizeTestProject", "test\E2ETests\TestAssets\DoNotParallelizeTestProject\DoNotParallelizeTestProject.csproj", "{8080DE48-CFD9-4F33-908A-8B71108CA223}"
 EndProject
-Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "CompatTestProject", "test\E2ETests\TestAssets\CompatTestProject\CompatTestProject.csproj", "{2D2C5B73-F1F1-47C8-BC5C-A172E9BB3D16}"
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "CompatTestProject", "test\E2ETests\TestAssets\CompatTestProject\CompatTestProject.csproj", "{2D2C5B73-F1F1-47C8-BC5C-A172E9BB3D16}"


Consider having separate sln for test assets.

smadala · 2018-05-07T12:19:16Z

src/Adapter/PlatformServices.Desktop/Services/DesktopTestSourceHost.cs

@@ -70,40 +68,108 @@ internal TestSourceHost(string sourceFileName, IRunSettings runSettings, IFramew
            this.SetContext(sourceFileName);
        }

+        public AppDomain AppDomain


Make it internal.

smadala · 2018-05-07T12:27:13Z

src/Adapter/PlatformServices.Desktop/Services/DesktopTestSourceHost.cs

        /// <summary>
        /// Setup the isolation host.
        /// </summary>
        public void SetupHost()
        {
+            List<string> resolutionPaths = this.GetResolutionPaths(this.sourceFileName, VSInstallationUtilities.IsCurrentProcessRunningInPortableMode());
+
+            EqtTrace.Info("DesktopTestSourceHost.SetupHost(): Creating assembly resolver with resolution paths {0}.", string.Join(",", resolutionPaths.ToArray()));


Check if tracing enabled?

smadala · 2018-05-07T12:40:44Z

src/Adapter/PlatformServices.Desktop/Services/DesktopTestSourceHost.cs

+
+                if (adapterSettings != null)
+                {
+                    try


smadala · 2018-05-07T12:53:14Z

test/ComponentTests/PlatformServices.Desktop.Component.Tests/DesktopTestSourceHostTests.cs

+            var mockRunSettings = new Mock<IRunSettings>();
+            mockRunSettings.Setup(rs => rs.SettingsXml).Returns(runSettingxml);
+
+            StringReader stringReader = new StringReader(runSettingxml);


smadala · 2018-05-07T12:58:01Z

...entTests/TestAssets/TestProjectForAssemblyResolution/TestProjectForAssemblyResolution.csproj

+    <IsPackable>false</IsPackable>
+    <AppendTargetFrameworkToOutputPath>false</AppendTargetFrameworkToOutputPath>
+    <OutputPath>$(TestFxRoot)artifacts\TestAssets\ComponentTests</OutputPath>
+    <SignAssembly>true</SignAssembly>


Remove this if not required?

Required. SampleFrameworkExtensions also has it. PlatformServices.Desktop.Component.Tests throws strong name error if key.snk is not referenced in TestProjectForAssemblyResolution.

jayaranigarg · 2018-05-08T05:46:53Z

@dotnet-bot Test Windows / Debug Build please

jayaranigarg · 2018-05-09T14:29:50Z

Fixes #384 and #356

Re-fix Compatibility issue of resetting appbase

6ca4519

jayaranigarg requested review from smadala, cltshivash and singhsarab April 19, 2018 10:24

jayaranigarg changed the title ~~Re-fix Compatibility issue of resetting appbase~~ Adding Resolution Paths during appdomain creation (Compatibility Isuue) Apr 19, 2018

singhsarab reviewed Apr 19, 2018

View reviewed changes

smadala reviewed Apr 19, 2018

View reviewed changes

cltshivash mentioned this pull request Apr 24, 2018

Detecting and warning the user about presence of multiple adapter versions #356

Closed

jayaranigarg added 4 commits May 4, 2018 19:29

Resolving PR comments and Adding UnitTests and ComponentTests

b1a770e

Merge branch 'master' into compatrefix

8366bd5

Fixing test

62452c7

Merge branch 'compatrefix' of https://github.com/jayaranigarg/testfx …

c855800

…into compatrefix

smadala approved these changes May 7, 2018

View reviewed changes

PR comments

b8466d8

jayaranigarg merged commit 45f5af0 into microsoft:master May 8, 2018

jayaranigarg deleted the compatrefix branch May 14, 2018 06:46


		this.targetFrameworkVersion = this.GetTargetFrameworkVersionString(this.sourceFileName);
		// Adding extensions folder to resolution paths

Adding Resolution Paths during appdomain creation (Compatibility Isuue) #404

Adding Resolution Paths during appdomain creation (Compatibility Isuue) #404

Conversation

jayaranigarg commented Apr 19, 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

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayaranigarg commented May 8, 2018

jayaranigarg commented May 9, 2018

jayaranigarg commented Apr 19, 2018 •

edited

Loading