New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abstracting Assembly Resolver #913

Merged
merged 11 commits into from Jul 19, 2017

Conversation

Projects
None yet
5 participants
@mayankbansal018
Contributor

mayankbansal018 commented Jul 5, 2017

Instantiating EqtTrace to do tracing in Platform Abstraction
follow up UAP implementation of Assembly Resolver

Abstracting Assembly Resolver
 Instantiating EqtTrace to do tracing in Platform Abstraction
 follow up UAP implementation of Assembly Resolver

@mayankbansal018 mayankbansal018 self-assigned this Jul 5, 2017

@mayankbansal018 mayankbansal018 requested a review from codito Jul 5, 2017

@mayankbansal018 mayankbansal018 requested a review from Faizan2304 Jul 5, 2017

Show outdated Hide outdated ...oft.TestPlatform.PlatformAbstractions/netcore/System/AssemblyResolver.cs
AssemblyLoadContext.Default.Resolving += this.CurrentDomainAssemblyResolve;
}
private Assembly CurrentDomainAssemblyResolve(AssemblyLoadContext loadContext, AssemblyName args)

This comment has been minimized.

@Faizan2304

Faizan2304 Jul 6, 2017

Contributor

Can we move all the common code of netcore/System/AssemblyResolver.cs and net46/System/AssemblyResolver.cs in common\System.AssemblyResolver.cs

@Faizan2304

Faizan2304 Jul 6, 2017

Contributor

Can we move all the common code of netcore/System/AssemblyResolver.cs and net46/System/AssemblyResolver.cs in common\System.AssemblyResolver.cs

@Faizan2304

This comment has been minimized.

Show comment
Hide comment
@Faizan2304

Faizan2304 Jul 6, 2017

Contributor

Please also add me in the PR where you will use this AsemblyResolver.

Contributor

Faizan2304 commented Jul 6, 2017

Please also add me in the PR where you will use this AsemblyResolver.

Show outdated Hide outdated src/Microsoft.TestPlatform.CoreUtilities/Tracing/EqtTrace.cs
@@ -23,7 +23,7 @@ namespace Microsoft.VisualStudio.TestPlatform.ObjectModel
/// </summary>
public static class EqtTrace
{
private static IPlatformEqtTrace traceImpl = new PlatformEqtTrace();
private static IPlatformEqtTrace traceImpl = PlatformEqtTrace.Instance;

This comment has been minimized.

@codito

codito Jul 14, 2017

Contributor

Not sure if this change is saving anything, and it introduces a singleton(state) on a platform abstraction API.

@codito

codito Jul 14, 2017

Contributor

Not sure if this change is saving anything, and it introduces a singleton(state) on a platform abstraction API.

Show outdated Hide outdated ...TestPlatform.PlatformAbstractions/Interfaces/System/IAssemblyResolver.cs
/// </summary>
public interface IAssemblyResolver : IDisposable
{
void SetCurrentDomainAssemblyResolve();

This comment has been minimized.

@codito

codito Jul 14, 2017

Contributor

Require documentation.

@codito

codito Jul 14, 2017

Contributor

Require documentation.

Show outdated Hide outdated src/Microsoft.TestPlatform.Common/ExtensionFramework/TestPluginCache.cs
@@ -501,55 +486,14 @@ private void SetupAssemblyResolver(string extensionAssembly)
// Add assembly resolver which can resolve the extensions from the specified directory.
if (this.assemblyResolver == null)
{
this.assemblyResolver = new AssemblyResolver(resolutionPaths);
this.assemblyResolver = new AssemblyResolver(resolutionPaths, EqtTrace.PlatformTrace);

This comment has been minimized.

@codito

codito Jul 14, 2017

Contributor

AssemblyResolver just requires a way to LogMessage(level, message). Why pass the platform trace to it?

@codito

codito Jul 14, 2017

Contributor

AssemblyResolver just requires a way to LogMessage(level, message). Why pass the platform trace to it?

Show outdated Hide outdated src/Microsoft.TestPlatform.CoreUtilities/Tracing/EqtTrace.cs
@@ -64,6 +64,14 @@ public static PlatformTraceLevel TraceLevel
}
#endif
public static IPlatformEqtTrace PlatformTrace

This comment has been minimized.

@codito

codito Jul 14, 2017

Contributor

We don't need to provide this.

@codito

codito Jul 14, 2017

Contributor

We don't need to provide this.

This comment has been minimized.

@mayankbansal018

mayankbansal018 Jul 19, 2017

Contributor

resolved

@mayankbansal018

mayankbansal018 Jul 19, 2017

Contributor

resolved

Show outdated Hide outdated ...TestPlatform.PlatformAbstractions/Interfaces/System/IAssemblyResolver.cs
/// <summary>
/// The AssemblyResolver interface.
/// </summary>
public interface IAssemblyResolver : IDisposable

This comment has been minimized.

@codito

codito Jul 14, 2017

Contributor

Assembly resolution is probably a runtime aspect (and not a system wide aspect). Suggest to move to Interfaces/Runtime.

@codito

codito Jul 14, 2017

Contributor

Assembly resolution is probably a runtime aspect (and not a system wide aspect). Suggest to move to Interfaces/Runtime.

This comment has been minimized.

@mayankbansal018

mayankbansal018 Jul 19, 2017

Contributor

resolved

@mayankbansal018

mayankbansal018 Jul 19, 2017

Contributor

resolved

Show outdated Hide outdated ...stPlatform.PlatformAbstractions/common/System/AssemblyResolverHandler.cs
EqtTrace.Info("AssemblyResolver: {0}: Failed to load assembly. Reason:{1} ", args.Name, ex);
WriteToEqtTrace("AssemblyResolver: {0}: Failed to load assembly. Reason:{1} ", assemblyName, ex);

This comment has been minimized.

@codito

codito Jul 14, 2017

Contributor

nit: remove extra space

@codito

codito Jul 14, 2017

Contributor

nit: remove extra space

Show outdated Hide outdated ...osoft.TestPlatform.PlatformAbstractions/net46/System/AssemblyResolver.cs
/// </returns>
[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1031:DoNotCatchGeneralExceptionTypes")]
[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Reliability", "CA2001:AvoidCallingProblematicMethods", MessageId = "System.Reflection.Assembly.LoadFrom")]
private Assembly AssemblyResolverEvent(object sender, object eventArgs)

This comment has been minimized.

@codito

codito Jul 14, 2017

Contributor

nit: method ordering is not correct. Isn't stylecop running for platform abstractions?

@codito

codito Jul 14, 2017

Contributor

nit: method ordering is not correct. Isn't stylecop running for platform abstractions?

Show outdated Hide outdated src/Microsoft.TestPlatform.Common/ExtensionFramework/TestPluginCache.cs
@@ -151,17 +154,15 @@ public bool LoadOnlyWellKnownExtensions
Dictionary<string, TPluginInfo> pluginInfos = null;
this.SetupAssemblyResolver(null);
this.platformAssemblyResolver = new PlatformAssemblyResolver();

This comment has been minimized.

@smadala

smadala Jul 17, 2017

Member

Move this line to below comment.

@smadala

smadala Jul 17, 2017

Member

Move this line to below comment.

This comment has been minimized.

@mayankbansal018

mayankbansal018 Jul 19, 2017

Contributor

resolved

@mayankbansal018

mayankbansal018 Jul 19, 2017

Contributor

resolved

@@ -46,6 +47,8 @@ public class TestPluginCache
/// </summary>
private AssemblyResolver assemblyResolver;
private IAssemblyResolver platformAssemblyResolver;

This comment has been minimized.

@smadala

smadala Jul 17, 2017

Member

Create assemblyResolver objects in internal .ctor, to enable mocking in unit tests.

@smadala

smadala Jul 17, 2017

Member

Create assemblyResolver objects in internal .ctor, to enable mocking in unit tests.

This comment has been minimized.

@codito

codito Jul 17, 2017

Contributor

Prefer protected since internal will enable other folks to directly use breaking the singleton usage.

@codito

codito Jul 17, 2017

Contributor

Prefer protected since internal will enable other folks to directly use breaking the singleton usage.

This comment has been minimized.

@mayankbansal018

mayankbansal018 Jul 19, 2017

Contributor

cannot create in .ctor, since on creation of this object we immediately assign a handler to current domains AssemblyResolve event.

The order of assigning event handler matters since the call to them go in that order itself

@mayankbansal018

mayankbansal018 Jul 19, 2017

Contributor

cannot create in .ctor, since on creation of this object we immediately assign a handler to current domains AssemblyResolve event.

The order of assigning event handler matters since the call to them go in that order itself

@codito

codito approved these changes Jul 17, 2017

Show outdated Hide outdated src/Microsoft.TestPlatform.Common/ExtensionFramework/TestPluginCache.cs
@@ -9,16 +9,17 @@ namespace Microsoft.VisualStudio.TestPlatform.Common.ExtensionFramework
using System.Linq;
using System.Reflection;
#if NET46

This comment has been minimized.

@codito

codito Jul 17, 2017

Contributor

Why do we need System.Threading for net46 only?

@codito

codito Jul 17, 2017

Contributor

Why do we need System.Threading for net46 only?

@@ -46,6 +47,8 @@ public class TestPluginCache
/// </summary>
private AssemblyResolver assemblyResolver;
private IAssemblyResolver platformAssemblyResolver;

This comment has been minimized.

@codito

codito Jul 17, 2017

Contributor

Prefer protected since internal will enable other folks to directly use breaking the singleton usage.

@codito

codito Jul 17, 2017

Contributor

Prefer protected since internal will enable other folks to directly use breaking the singleton usage.

this.platformAssemblyResolver = new PlatformAssemblyResolver();

This comment has been minimized.

@codito

codito Jul 17, 2017

Contributor

Please make it testable.

@codito

codito Jul 17, 2017

Contributor

Please make it testable.

This comment has been minimized.

@codito

codito Jul 17, 2017

Contributor

Given there is a PlatformAssemblyResolver, I think we should add unit tests for the business logic in TestPluginCache and AssemblyResolver.

@codito

codito Jul 17, 2017

Contributor

Given there is a PlatformAssemblyResolver, I think we should add unit tests for the business logic in TestPluginCache and AssemblyResolver.

Show outdated Hide outdated src/Microsoft.TestPlatform.Common/ExtensionFramework/TestPluginCache.cs
this.platformAssemblyResolver.AssemblyResolve -= this.CurrentDomainAssemblyResolve;
this.platformAssemblyResolver.Dispose();
}

This comment has been minimized.

@codito

codito Jul 17, 2017

Contributor

nit: remove spaces

@codito

codito Jul 17, 2017

Contributor

nit: remove spaces

Show outdated Hide outdated ...crosoft.TestPlatform.PlatformAbstractions/Interfaces/System/IAssembly.cs
public interface IAssembly
{
/// <summary>
/// Gets the loation of current assembly

This comment has been minimized.

@codito

codito Jul 17, 2017

Contributor

nit: spelling of location

@codito

codito Jul 17, 2017

Contributor

nit: spelling of location

This comment has been minimized.

@mayankbansal018

mayankbansal018 Jul 19, 2017

Contributor

resolved

@mayankbansal018

mayankbansal018 Jul 19, 2017

Contributor

resolved

Show outdated Hide outdated ...soft.TestPlatform.PlatformAbstractions/common/System/PlatformAssembly.cs
}
/// <inheritdoc/>
public AssemblyName GetAssemblyNameFromPath(string assemblyPath)

This comment has been minimized.

@codito

codito Jul 17, 2017

Contributor

This can separate into two implementations

@codito

codito Jul 17, 2017

Contributor

This can separate into two implementations

This comment has been minimized.

@mayankbansal018

mayankbansal018 Jul 19, 2017

Contributor

resolved

@mayankbansal018

mayankbansal018 Jul 19, 2017

Contributor

resolved

Show outdated Hide outdated ...stPlatform.PlatformAbstractions/net46/System/PlatformAssemblyResolver.cs
/// <summary>
/// Initializes a new instance of the <see cref="PlatformAssemblyResolver"/> class.
/// </summary>
[System.Security.SecurityCritical]

This comment has been minimized.

@codito

codito Jul 17, 2017

Contributor

nit: remove these.

@codito

codito Jul 17, 2017

Contributor

nit: remove these.

This comment has been minimized.

@Faizan2304

Faizan2304 Jul 19, 2017

Contributor

This also

@Faizan2304

Faizan2304 Jul 19, 2017

Contributor

This also

Show outdated Hide outdated ...stPlatform.PlatformAbstractions/net46/System/PlatformAssemblyResolver.cs
/// <summary>
/// Specifies whether the resolver is disposed or not
/// </summary>
private bool isDisposed;

This comment has been minimized.

@codito

codito Jul 17, 2017

Contributor

nit: disposed instead of isDisposed.

@codito

codito Jul 17, 2017

Contributor

nit: disposed instead of isDisposed.

This comment has been minimized.

@Faizan2304

Faizan2304 Jul 19, 2017

Contributor

@mayankbansal018 are we not fixing it?

@Faizan2304

Faizan2304 Jul 19, 2017

Contributor

@mayankbansal018 are we not fixing it?

Show outdated Hide outdated ...oft.TestPlatform.PlatformAbstractions/uap10.0/System/PlatformAssembly.cs
/// <inheritdoc/>
public class PlatformAssembly : IAssembly
{
public string GetAssemblyLocation(Assembly assembly)

This comment has been minimized.

@codito

codito Jul 17, 2017

Contributor

nit: inheritdoc

@codito

codito Jul 17, 2017

Contributor

nit: inheritdoc

This comment has been minimized.

@mayankbansal018

mayankbansal018 Jul 19, 2017

Contributor

resolved

@mayankbansal018

mayankbansal018 Jul 19, 2017

Contributor

resolved

Show outdated Hide outdated ...Platform.PlatformAbstractions/uap10.0/System/PlatformAssemblyResolver.cs
private void DummyEventThrower()
{
this.AssemblyResolve(this, null);

This comment has been minimized.

@codito

codito Jul 17, 2017

Contributor

Add a comment this is required to keep compiler happy.

@codito

codito Jul 17, 2017

Contributor

Add a comment this is required to keep compiler happy.

This comment has been minimized.

@mayankbansal018

mayankbansal018 Jul 19, 2017

Contributor

resolved

@mayankbansal018

mayankbansal018 Jul 19, 2017

Contributor

resolved

mayankbansal018 added some commits Jul 19, 2017

@mayankbansal018 mayankbansal018 merged commit b91bd79 into Microsoft:master Jul 19, 2017

4 checks passed

Ubuntu16.04 / Debug Build Build finished.
Details
Ubuntu16.04 / Release Build Build finished.
Details
Windows_NT / Debug Build Build finished.
Details
Windows_NT / Release Build Build finished.
Details

@mayankbansal018 mayankbansal018 deleted the mayankbansal018:assembly-abstraction branch Jul 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment