Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

refactoring and more warnings from TypeCatalogue #261

Merged
merged 7 commits into from

3 participants

@adamralph
Owner

Some opinionated refactorings and I've added warnings when loading badly formatted assemblies (which includes non-managed assemblies) and when failing to load all types from loaded assemblies.

I've also changed TypeCatalogue so that the constructor doesn't do all the work with the filesystem etc. which I think is a bad pattern. That work now occurs in a Load() method.

Each commit is quite clear in it's purpose so can be reviewed separately if that's easier.

All open to discussion.

@blairconrad blairconrad commented on the diff
Source/FakeItEasy/Core/TypeCatalogue.cs
@@ -11,9 +10,8 @@
/// Provides access to all types in:
/// <list type="bullet">
/// <item>FakeItEasy,</item>
- /// <item>AppDomain assemblies that reference FakeItEasy, and</item>
- /// <item>assembly files whose paths are supplied to the class constructor, and
- /// that also reference FakeItEasy.</item>
+ /// <item>assemblies loaded into the current <see cref="AppDomain"/> that reference FakeItEasy and</item>
@blairconrad Owner

Have we talked about the Oxford comma before? I see you removed one. Is our preferred style to omit them?

@adamralph Owner

I didn't know about the Oxford comma until you pointed it out to me, and just had to remind myself what it is again :wink:.

I was taught to write without it so I naturally omit it but I guess we just ought to be consistent. What are we doing elsewhere?

@blairconrad Owner

It's hard to make a definitive statement, but it appears that we are not using the Oxford comma, but I have been, mostly in my recent PR for #130. :blush:
I'm happy to (try to) conform.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@blairconrad blairconrad commented on the diff
Source/FakeItEasy/Core/TypeCatalogue.cs
((47 lines not shown))
}
- catch (BadImageFormatException)
+ catch (Exception ex)
@blairconrad Owner

I think I understand the goal here—you're worried about not being able to load assemblies for some reason, and now that users can specify assembly names explicitly, you want to provide them with helpful messages when we can't.
I'm concerned, though, that this change will result in a lot of error messages whenever an unmanaged assembly is specified in the list of extra assemblies (or is in the working directory, if we're using the DefaultBootstrapper). I think we should avoid complaining about unmanaged assemblies if at all possible, to avoid alarming people for no good reason, especially given that those assemblies couldn't possible contain extensions anyhow.

@adamralph Owner

Yeah, now that people can pass in explicit paths I thought it better to always warn.

I see your point about the default bootstrapper but I'm also uneasy with silently swallowing this exception. All this exception tells us is that the assembly is not a managed assembly. It could be a blank file, text file, etc. which happens, by some turn of events, to have been given a .dll extension. There could also be a scenario where a managed assembly file has been corrupted in some way and therefore not used when the user expects it to be used.

Is there a reliable way of telling whether an assembly is unmanaged without trying to load it and catching this exception? If so, we could suppress those from the output by avoiding the load altogether and then let the warning output handle all other cases.

@blairconrad Owner

As I understand it, there are ways. A StackOverflow question pointed me to some code lifted from the Galio project. I've not tried it.

For one of the projects I work on at the Day Job, there are quite a few managed DLLs, and there'd be quite a lot of errors, and I worry that others may have the same experience. However, it's possible that mixed native/managed DLLs aren't a big problem in general, and I don't want to make a big deal over this if it's just my niche problem.

@adamralph Owner

The Galio code is very interesting! Do we think this is worth doing?

@blairconrad Owner

It is interesting. Worth doing? Well, I think I'm on the "no" side.
I think we're getting off into corner cases on corner cases. You say that sometimes we have a *.dll file that is blank or a text file. I believe it happens, but it must be exceedingly rare. If we're looking at someone expecting an extension to be found in that file, I think they would quickly realize that the file is not a real DLL. I hope.
I think the corrupt file is a bigger concern, but I wonder how often this happens as well.

Honestly, the more we talk and think about it, the more I long for the breaking "don't scan the working directory" change that we've talked about in the past. It does obviate an awful lot of problems, including this one and the possible slow startup out of the box.

Rather than looking at implementing a somewhat complicated managed/unmanged or 32/64 bit detector in order to provide better error messages for code paths that I expect the vast majority of users don't know or care about, perhaps we should consider whether we ultimately want to make the breaking change and then schedule it in. With the bootstrapper we at least have a way forward for those few who may want to load extensions from outside the app domain, and then I'd feel much better about complaining about any assembly that failed to load, because the client took the time to ask for it to be loaded.

@adamralph Owner

Agreed on the breaking change. I've created #268 to track it, marked as breaking and assigned to the new 2.0 milestone.

I think for 1.x, I'd prefer to have failures to load assemblies logged in stdout rather than silent swallowing. The good is thing is that with the new bootstrapper, anyone who has a large number of unmanaged assemblies in their output folder can already override the behaviour today and either suppress all file system scanning or pass in the path of a specific assembly containing extensions.

@blairconrad Owner

Good point about using the bootstrapper to tailor the scanning if the stdout warnings become a problem. Heck, once I get 1.18.0 at the Day Job, I'm planning on throwing a custom bootstrapper in anyhow, if only for the (minimal) speed boost.

I think this plans avoids the need for the complicated managed/unmanaged detector. I'll give the current code another quick read, but I think I'm happy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Source/FakeItEasy/Core/TypeCatalogue.cs
((89 lines not shown))
}
- private static void WarnFailedToLoadAssembly(string assemblyFile, Exception e)
+ private static void WarnFailedToLoadAssembly(string assemblyText, Exception ex)
@blairconrad Owner

assemblyText doesn't sound so much like the path to the assembly as it does the contents…

@adamralph Owner

The reason I changed it is because in this change set, I now use the parameter for passing in either the path to an assembly or it's name (when loading types from it). We could change it to assemblyPathOrName?

@blairconrad Owner

I missed that. I do prefer assemblyPathOrName as a parameter name.

@adamralph Owner

OK, no problem. I'll change it.

@blairconrad Owner

On closer inspection, I'm not sure why we ever need to pass an assembly name into WarnFailedToLoadAssembly. On line 41, if we catch an exception, it's during enumeration of the assembly types, not when loading the assemblies. I support switching away from the "ignore the exception" approach, but I don't think reporting a "FailedToLoadAssembly" is necessarily the way to go.

@adamralph Owner

Good call. We ought to report 'failed to retrieve types from assembly XXX' for this case. I'll write a separate method and use that instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@blairconrad blairconrad commented on the diff
Source/FakeItEasy/Core/TypeCatalogue.cs
((12 lines not shown))
[SuppressMessage("Microsoft.Design", "CA1031:DoNotCatchGeneralExceptionTypes", Justification = "Defensive and performed on best effort basis.")]
- public TypeCatalogue(IEnumerable<string> assemblyFilesToScan)
+ public void Load(IEnumerable<string> extraAssemblyFiles)
@blairconrad Owner

In general, I'm a fan of having newly-created objects be usable right away.
I understand that Load is a little more descriptive than passing the paths to the constructor, but I feel that it might not be worth it, especially given that we've traded a constructor for a constructor plus a method that must be called before the object can be used.

Of course, now I'm afraid we just have duelling opinions.

@adamralph Owner

If we can't agree on this then I'll just back down since no one has complained about the current design until now :wink:.

The reason I did this is that I don't believe a constructor has any business performing I/O internally. All it should do is create a valid initial state for the object. If we were passing in some kind of dependency as a provider interface then the implementation passed in would be at liberty to do whatever it likes but that is OK, since the caller of the constructor is responsible for the argument and any consequences that has, but I don't believe a constructor should ever encapsulate I/O operations itself.

@philippdolder Owner

I agree with @adamralph.
I dislike constructors doing anything else than initializing fields:

  • create new collections
  • assign parameters to fields.

I'm a purist in that matter. e.g. nothing that could go wrong is allowed in constructors. No call to factory methods, no whatever.

But I'm also aware that this is not the case everywhere in our code.

@blairconrad Owner

I'm happy to go along with the Load method. My objection is weakened by the fact that only FakeItEasy is likely to be using the TypeCatalogue, even though it's public for some reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Source/FakeItEasy/Core/TypeCatalogue.cs
((93 lines not shown))
{
- var outputWriter = new DefaultOutputWriter(Console.Write);
- outputWriter.Write(
+ var writer = new DefaultOutputWriter(Console.Write);
+ writer.Write(
"Warning: FakeItEasy failed to load assembly '{0}' while scanning for extension points. Any IArgumentValueFormatters, IDummyDefinitions, and IFakeConfigurators in that assembly will not be available.",
@adamralph Owner

Come to think of it 'whilst scanning for extension points' is a lie in this context. This is just a type catalogue. It has no knowledge whatsoever of how those types will be used.

@blairconrad Owner

Absolutely true. Of course, the only thing we use the TypeCatalogue for is to find extension points.
Unfortunately, that doesn't help us out much. Having the log message know more than it should is distasteful, but I think we want to tell the user what impact not being able to load an assembly might have.
Rename the TypeCatalogue?
Hmm… that was said half in jest, but I think it's not a bad way forward.
I kind of think the putting the TypeCatalogue in the container just so the TypeCatalogueInstanceProvider can get it out seems to me to be a little over-engineered anyhow. Changing to a more targeted name and keeping its usage all in the ImportsModule may save headaches.

@adamralph Owner

Hmmm... you might be onto something with over engineering comment. I'm suspecting there is a way we could simplify all this which would bring the assembly loading and type retrieval right into the context of extension scanning where it seems to belong...

@blairconrad Owner

Okay. I think this is the only outstanding point. I think there's no rush on this.
If you're agreed, I'll hit the "merge" button (or you could, if you're up early and anxious), and we can pursue it separately. I think that'll a lot clearer than continuing here.
Of course, if you'd rather continue in this PR, I'm game.

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

Hmm. I responded to a comment in a line note, but it's collapsed by default, so I'm not sure you'll see it. Just in case:

Come to think of it 'whilst scanning for extension points' is a lie in this context. This is just a type catalogue. It has no knowledge whatsoever of how those types will be used.

Absolutely true. Of course, the only thing we use the TypeCatalogue for is to find extension points.
Unfortunately, that doesn't help us out much. Having the log message know more than it should is distasteful, but I think we want to tell the user what impact not being able to load an assembly might have.
Rename the TypeCatalogue?
Hmm… that was said half in jest, but I think it's not a bad way forward.
I kind of think that putting the TypeCatalogue in the container just so the TypeCatalogueInstanceProvider can get it out seems to me to be a little over-engineered anyhow. >>Changing to a more targeted name and keeping its usage all in the ImportsModule may save headaches.

Hmmm... you might be onto something with over engineering comment. I'm suspecting there is a way we could simplify all this which would bring the assembly loading and type retrieval right into the context of extension scanning where it seems to belong...

Okay. I think this is the only outstanding point. As far as I'm concerned, there's no rush on this.
If you're agreed, I'll hit the "merge" button (or you could, if you're up early and anxious), and we can pursue it separately. I think that'll a lot clearer than continuing here.
Of course, if you'd rather continue in this PR, I'm game.

@adamralph
Owner

I agree, I think the relationship between ImportsModule, TypeCatalogue and TypeCatalogueInstanceProvider can be pursued separately as a further refactoring if we think it's worth it and, as you say there's no rush on it anyway.

@blairconrad blairconrad merged commit 486ac1c into from
@adamralph adamralph deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
146 Source/FakeItEasy.IntegrationTests/TypeCatalogueTests.cs
@@ -6,93 +6,153 @@ namespace FakeItEasy.IntegrationTests
using System.IO;
using System.Linq;
using FakeItEasy.Core;
- using FakeItEasy.Tests.TestHelpers;
using FluentAssertions;
using NUnit.Framework;
- [TestFixture]
public class TypeCatalogueTests
{
- private TypeCatalogue catalogue;
-
- [TestFixtureSetUp]
- public void TestFixtureSetup()
- {
- this.catalogue = new TypeCatalogue(Directory.GetFiles(Environment.CurrentDirectory, "*.dll"));
- }
-
[SuppressMessage("Microsoft.Usage", "CA2202:Do not dispose objects multiple times", Justification = "No exception is thrown.")]
[Test]
public void Should_warn_of_duplicate_input_assemblies_with_different_paths()
{
- string actualMessage;
-
// Arrange
- var originalDirectory = Environment.CurrentDirectory;
- var originalDirectoryName = new DirectoryInfo(originalDirectory).Name;
- var originalStandardOut = Console.Out;
- Exception exception;
+ var currentDirectoryName = new DirectoryInfo(Environment.CurrentDirectory).Name;
+
+ // FakeItEasy.IntegrationTests.External has copies of many of the assemblies used in these
+ // tests as well. By specifying assembly paths from that directory, the catalog will see
+ // those assemblies in both locations, and should fail to load the duplicates.
+ var directoryToScan = Path.Combine(
+ Environment.CurrentDirectory,
+ Path.Combine(@"..\..\..\FakeItEasy.IntegrationTests.External\bin", currentDirectoryName));
+
+ var expectedMessageFormat =
+@"*Warning: FakeItEasy failed to load assembly '*FakeItEasy.IntegrationTests.External\bin\{0}\FakeItEasy.IntegrationTests.External.dll' while scanning for extension points. Any IArgumentValueFormatters, IDummyDefinitions, and IFakeConfigurators in that assembly will not be available.
+ API restriction: The assembly '*FakeItEasy.IntegrationTests.External\bin\{0}\FakeItEasy.IntegrationTests.External.dll' has already loaded from a different location. It cannot be loaded from a new location within the same appdomain.*";
+
+ var expectedMessage = string.Format(CultureInfo.InvariantCulture, expectedMessageFormat, currentDirectoryName);
+ string actualMessage;
- try
+ using (var messageStream = new MemoryStream())
+ using (var messageWriter = new StreamWriter(messageStream))
{
- // FakeItEasy.IntegrationTests.External has copies of many of the assemblies used in these
- // tests as well. By specifying assembly paths from that directory, the catalog will see
- // those assemblies in both locations, and should fail to load the duplicates.
- var directoryToScan = Path.Combine(
- Environment.CurrentDirectory,
- Path.Combine(@"..\..\..\FakeItEasy.IntegrationTests.External\bin", originalDirectoryName));
-
- using (var messageStream = new MemoryStream())
- using (var messageWriter = new StreamWriter(messageStream))
- {
- Console.SetOut(messageWriter);
+ var catalogue = new TypeCatalogue();
+ var originalWriter = Console.Out;
+ Console.SetOut(messageWriter);
+ try
+ {
// Act
- exception = Record.Exception(() => new TypeCatalogue(Directory.GetFiles(directoryToScan, "*.dll")));
- messageWriter.Flush();
- actualMessage = messageWriter.Encoding.GetString(messageStream.GetBuffer());
+ catalogue.Load(Directory.GetFiles(directoryToScan, "*.dll"));
+ }
+ finally
+ {
+ Console.SetOut(originalWriter);
}
+
+ messageWriter.Flush();
+ actualMessage = messageWriter.Encoding.GetString(messageStream.GetBuffer());
}
- finally
+
+ // Assert
+ actualMessage.Should().Match(expectedMessage);
+ }
+
+ [SuppressMessage("Microsoft.Usage", "CA2202:Do not dispose objects multiple times", Justification = "No exception is thrown.")]
+ [Test]
+ public void Should_warn_of_bad_assembly_files()
+ {
+ // Arrange
+ var badAssemblyFile = Path.GetTempFileName() + ".dll";
+ var expectedMessageFormat =
+@"*Warning: FakeItEasy failed to load assembly '{0}' while scanning for extension points. Any IArgumentValueFormatters, IDummyDefinitions, and IFakeConfigurators in that assembly will not be available.
+ *{0}*";
+
+ var expectedMessage = string.Format(CultureInfo.InvariantCulture, expectedMessageFormat, badAssemblyFile);
+ string actualMessage = null;
+
+ using (var stream = new MemoryStream())
+ using (var writer = new StreamWriter(stream))
{
- Console.SetOut(originalStandardOut);
+ var catalogue = new TypeCatalogue();
+
+ File.CreateText(badAssemblyFile).Close();
+ try
+ {
+ var originalWriter = Console.Out;
+ Console.SetOut(writer);
+ try
+ {
+ // Act
+ catalogue.Load(new[] { badAssemblyFile });
+ }
+ finally
+ {
+ Console.SetOut(originalWriter);
+ }
+ }
+ finally
+ {
+ File.Delete(badAssemblyFile);
+ }
+
+ writer.Flush();
+ actualMessage = writer.Encoding.GetString(stream.GetBuffer());
}
// Assert
- exception.Should().BeNull();
-
- const string ExpectedMessageFormat = @"*Warning: FakeItEasy failed to load assembly '*FakeItEasy.IntegrationTests.External\bin\{0}\FakeItEasy.IntegrationTests.External.dll' while scanning for extension points. Any IArgumentValueFormatters, IDummyDefinitions, and IFakeConfigurators in that assembly will not be available.
- API restriction: The assembly '*FakeItEasy.IntegrationTests.External\bin\{0}\FakeItEasy.IntegrationTests.External.dll' has already loaded from a different location. It cannot be loaded from a new location within the same appdomain.*";
- string expectedMessagePattern = string.Format(CultureInfo.InvariantCulture, ExpectedMessageFormat, originalDirectoryName);
- actualMessage.Should().Match(expectedMessagePattern);
+ actualMessage.Should().Match(expectedMessage);
}
[Test]
public void Should_be_able_to_get_types_from_fakeiteasy()
{
+ // Arrange
+ var catalogue = new TypeCatalogue();
+
+ // Act
+ catalogue.Load(Directory.GetFiles(Environment.CurrentDirectory, "*.dll"));
+
// Assert
- this.catalogue.GetAvailableTypes().Should().Contain(typeof(A));
+ catalogue.GetAvailableTypes().Should().Contain(typeof(A));
}
[Test]
public void Should_be_able_to_get_types_from_assembly_in_app_domain()
{
+ // Arrange
+ var catalogue = new TypeCatalogue();
+
+ // Act
+ catalogue.Load(Directory.GetFiles(Environment.CurrentDirectory, "*.dll"));
+
// Assert
- this.catalogue.GetAvailableTypes().Should().Contain(typeof(DoubleValueFormatter));
+ catalogue.GetAvailableTypes().Should().Contain(typeof(DoubleValueFormatter));
}
[Test]
public void Should_be_able_to_get_types_from_external_assembly_in_directory()
{
+ // Arrange
+ var catalogue = new TypeCatalogue();
+
+ // Act
+ catalogue.Load(Directory.GetFiles(Environment.CurrentDirectory, "*.dll"));
+
// Assert
- this.catalogue.GetAvailableTypes().Select(type => type.FullName).Should().Contain("FakeItEasy.IntegrationTests.External.GuidValueFormatter");
+ catalogue.GetAvailableTypes().Select(type => type.FullName).Should().Contain("FakeItEasy.IntegrationTests.External.GuidValueFormatter");
}
[Test]
public void Should_not_be_able_to_get_types_from_assembly_that_does_not_reference_fakeiteasy()
{
+ // Arrange
+ var catalogue = new TypeCatalogue();
+
+ // Act
+ catalogue.Load(Directory.GetFiles(Environment.CurrentDirectory, "*.dll"));
+
// Assert
- this.catalogue.GetAvailableTypes().Should().NotContain(typeof(string));
+ catalogue.GetAvailableTypes().Should().NotContain(typeof(string));
}
}
}
View
2  Source/FakeItEasy.Tests/NullGuardedConstraint.cs
@@ -304,7 +304,7 @@ private IEnumerable<CallThatShouldThrow> GetArgumentPermutationsThatShouldThrow(
{
var permutation = new CallThatShouldThrow();
permutation.ArgumentName = argument.Name;
- permutation.Arguments = this.ArgumentValues.Take(index).Concat(new object[] { null }).Concat(this.ArgumentValues.Skip(index + 1)).ToArray();
+ permutation.Arguments = this.ArgumentValues.Take(index).Concat(default(object)).Concat(this.ArgumentValues.Skip(index + 1)).ToArray();
result.Add(permutation);
}
View
109 Source/FakeItEasy/Core/TypeCatalogue.cs
@@ -3,7 +3,6 @@
using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
- using System.IO;
using System.Linq;
using System.Reflection;
@@ -11,9 +10,8 @@
/// Provides access to all types in:
/// <list type="bullet">
/// <item>FakeItEasy,</item>
- /// <item>AppDomain assemblies that reference FakeItEasy, and</item>
- /// <item>assembly files whose paths are supplied to the class constructor, and
- /// that also reference FakeItEasy.</item>
+ /// <item>assemblies loaded into the current <see cref="AppDomain"/> that reference FakeItEasy and</item>
@blairconrad Owner

Have we talked about the Oxford comma before? I see you removed one. Is our preferred style to omit them?

@adamralph Owner

I didn't know about the Oxford comma until you pointed it out to me, and just had to remind myself what it is again :wink:.

I was taught to write without it so I naturally omit it but I guess we just ought to be consistent. What are we doing elsewhere?

@blairconrad Owner

It's hard to make a definitive statement, but it appears that we are not using the Oxford comma, but I have been, mostly in my recent PR for #130. :blush:
I'm happy to (try to) conform.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ /// <item>assemblies whose paths are supplied to the constructor, that also reference FakeItEasy.</item>
/// </list>
/// </summary>
public class TypeCatalogue : ITypeCatalogue
@@ -22,13 +20,16 @@ public class TypeCatalogue : ITypeCatalogue
private readonly List<Type> availableTypes = new List<Type>();
/// <summary>
- /// Initializes a new instance of the <see cref="TypeCatalogue"/> class.
+ /// Loads the available types into the <see cref="TypeCatalogue"/>.
/// </summary>
- /// <param name="assemblyFilesToScan">The full paths to non-AppDomain assembly files from which to load types.</param>
+ /// <param name="extraAssemblyFiles">
+ /// The full paths to assemblies from which to load types,
+ /// as well as assemblies loaded into the current <see cref="AppDomain"/>.
+ /// </param>
[SuppressMessage("Microsoft.Design", "CA1031:DoNotCatchGeneralExceptionTypes", Justification = "Defensive and performed on best effort basis.")]
- public TypeCatalogue(IEnumerable<string> assemblyFilesToScan)
+ public void Load(IEnumerable<string> extraAssemblyFiles)
@blairconrad Owner

In general, I'm a fan of having newly-created objects be usable right away.
I understand that Load is a little more descriptive than passing the paths to the constructor, but I feel that it might not be worth it, especially given that we've traded a constructor for a constructor plus a method that must be called before the object can be used.

Of course, now I'm afraid we just have duelling opinions.

@adamralph Owner

If we can't agree on this then I'll just back down since no one has complained about the current design until now :wink:.

The reason I did this is that I don't believe a constructor has any business performing I/O internally. All it should do is create a valid initial state for the object. If we were passing in some kind of dependency as a provider interface then the implementation passed in would be at liberty to do whatever it likes but that is OK, since the caller of the constructor is responsible for the argument and any consequences that has, but I don't believe a constructor should ever encapsulate I/O operations itself.

@philippdolder Owner

I agree with @adamralph.
I dislike constructors doing anything else than initializing fields:

  • create new collections
  • assign parameters to fields.

I'm a purist in that matter. e.g. nothing that could go wrong is allowed in constructors. No call to factory methods, no whatever.

But I'm also aware that this is not the case everywhere in our code.

@blairconrad Owner

I'm happy to go along with the Load method. My objection is weakened by the fact that only FakeItEasy is likely to be using the TypeCatalogue, even though it's public for some reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
{
- foreach (var assembly in GetAllAvailableAssemblies(assemblyFilesToScan))
+ foreach (var assembly in GetAllAssemblies(extraAssemblyFiles))
{
try
{
@@ -37,8 +38,10 @@ public TypeCatalogue(IEnumerable<string> assemblyFilesToScan)
this.availableTypes.Add(type);
}
}
- catch
+ catch (Exception ex)
{
+ WarnFailedToGetTypes(assembly, ex);
+ continue;
}
}
}
@@ -53,74 +56,90 @@ public IEnumerable<Type> GetAvailableTypes()
}
[SuppressMessage("Microsoft.Design", "CA1031:DoNotCatchGeneralExceptionTypes", Justification = "Appropriate in try methods.")]
- private static IEnumerable<Assembly> GetAllAvailableAssemblies(IEnumerable<string> assemblyFilesToScan)
+ private static IEnumerable<Assembly> GetAllAssemblies(IEnumerable<string> extraAssemblyFiles)
{
- var appDomainAssemblies = AppDomain.CurrentDomain.GetAssemblies();
- var appDomainAssembliesReferencingFakeItEasy = appDomainAssemblies.Where(assembly => assembly.ReferencesFakeItEasy());
+ var loadedAssemblies = AppDomain.CurrentDomain.GetAssemblies();
+ var loadedAssembliesReferencingFakeItEasy = loadedAssemblies.Where(assembly => assembly.ReferencesFakeItEasy());
// Find the paths of already loaded assemblies so we don't double scan them.
- // Checking Assembly.IsDynamic would be preferable to the business with the Namespace, but the former isn't available in .NET 3.5.
+ // Checking Assembly.IsDynamic would be preferable to the business with the Namespace
+ // but the former isn't available in .NET 3.5.
// Exclude the ReflectionOnly assemblies because we want to be able to fully load them if we need to.
- var loadedAssemblyPaths = new HashSet<string>(
- appDomainAssemblies
+ var loadedAssemblyFiles = new HashSet<string>(
+ loadedAssemblies
.Where(a => !a.ReflectionOnly && a.ManifestModule.GetType().Namespace != "System.Reflection.Emit")
.Select(a => a.Location),
StringComparer.OrdinalIgnoreCase);
- var folderAssembliesReferencingFakeItEasy = new List<Assembly>();
-
// Skip assemblies already in the application domain.
- // This is an optimization that can be fooled by test runners that make shadow copies of the assemblies, but it's a start.
- foreach (var assemblyFile in assemblyFilesToScan.Except(loadedAssemblyPaths))
+ // This optimization can be fooled by test runners that make shadow copies of the assemblies but it's a start.
+ return GetAssemblies(extraAssemblyFiles.Except(loadedAssemblyFiles))
+ .Concat(loadedAssembliesReferencingFakeItEasy)
+ .Concat(FakeItEasyAssembly)
+ .Distinct();
+ }
+
+ private static IEnumerable<Assembly> GetAssemblies(IEnumerable<string> files)
+ {
+ foreach (var file in files)
{
- Assembly assembly = null;
+ Assembly reflectedAssembly = null;
try
{
- assembly = Assembly.ReflectionOnlyLoadFrom(assemblyFile);
+ reflectedAssembly = Assembly.ReflectionOnlyLoadFrom(file);
}
- catch (BadImageFormatException)
+ catch (Exception ex)
@blairconrad Owner

I think I understand the goal here—you're worried about not being able to load assemblies for some reason, and now that users can specify assembly names explicitly, you want to provide them with helpful messages when we can't.
I'm concerned, though, that this change will result in a lot of error messages whenever an unmanaged assembly is specified in the list of extra assemblies (or is in the working directory, if we're using the DefaultBootstrapper). I think we should avoid complaining about unmanaged assemblies if at all possible, to avoid alarming people for no good reason, especially given that those assemblies couldn't possible contain extensions anyhow.

@adamralph Owner

Yeah, now that people can pass in explicit paths I thought it better to always warn.

I see your point about the default bootstrapper but I'm also uneasy with silently swallowing this exception. All this exception tells us is that the assembly is not a managed assembly. It could be a blank file, text file, etc. which happens, by some turn of events, to have been given a .dll extension. There could also be a scenario where a managed assembly file has been corrupted in some way and therefore not used when the user expects it to be used.

Is there a reliable way of telling whether an assembly is unmanaged without trying to load it and catching this exception? If so, we could suppress those from the output by avoiding the load altogether and then let the warning output handle all other cases.

@blairconrad Owner

As I understand it, there are ways. A StackOverflow question pointed me to some code lifted from the Galio project. I've not tried it.

For one of the projects I work on at the Day Job, there are quite a few managed DLLs, and there'd be quite a lot of errors, and I worry that others may have the same experience. However, it's possible that mixed native/managed DLLs aren't a big problem in general, and I don't want to make a big deal over this if it's just my niche problem.

@adamralph Owner

The Galio code is very interesting! Do we think this is worth doing?

@blairconrad Owner

It is interesting. Worth doing? Well, I think I'm on the "no" side.
I think we're getting off into corner cases on corner cases. You say that sometimes we have a *.dll file that is blank or a text file. I believe it happens, but it must be exceedingly rare. If we're looking at someone expecting an extension to be found in that file, I think they would quickly realize that the file is not a real DLL. I hope.
I think the corrupt file is a bigger concern, but I wonder how often this happens as well.

Honestly, the more we talk and think about it, the more I long for the breaking "don't scan the working directory" change that we've talked about in the past. It does obviate an awful lot of problems, including this one and the possible slow startup out of the box.

Rather than looking at implementing a somewhat complicated managed/unmanged or 32/64 bit detector in order to provide better error messages for code paths that I expect the vast majority of users don't know or care about, perhaps we should consider whether we ultimately want to make the breaking change and then schedule it in. With the bootstrapper we at least have a way forward for those few who may want to load extensions from outside the app domain, and then I'd feel much better about complaining about any assembly that failed to load, because the client took the time to ask for it to be loaded.

@adamralph Owner

Agreed on the breaking change. I've created #268 to track it, marked as breaking and assigned to the new 2.0 milestone.

I think for 1.x, I'd prefer to have failures to load assemblies logged in stdout rather than silent swallowing. The good is thing is that with the new bootstrapper, anyone who has a large number of unmanaged assemblies in their output folder can already override the behaviour today and either suppress all file system scanning or pass in the path of a specific assembly containing extensions.

@blairconrad Owner

Good point about using the bootstrapper to tailor the scanning if the stdout warnings become a problem. Heck, once I get 1.18.0 at the Day Job, I'm planning on throwing a custom bootstrapper in anyhow, if only for the (minimal) speed boost.

I think this plans avoids the need for the complicated managed/unmanaged detector. I'll give the current code another quick read, but I think I'm happy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
{
- // The assembly may not be managed, so move on.
- continue;
- }
- catch (Exception e)
- {
- WarnFailedToLoadAssembly(assemblyFile, e);
+ WarnFailedToLoadAssembly(file, ex);
continue;
}
- if (!assembly.ReferencesFakeItEasy())
+ if (!reflectedAssembly.ReferencesFakeItEasy())
{
continue;
}
+ // A reflection-only loaded assembly can't be scanned for types, so fully load it before saving it.
+ Assembly loadedAssembly = null;
try
{
- // A reflection-only loaded assembly can't be scanned for types, so fully load it before saving it.
- folderAssembliesReferencingFakeItEasy.Add(Assembly.Load(assembly.GetName()));
+ loadedAssembly = Assembly.Load(reflectedAssembly.GetName());
}
- catch
+ catch (Exception ex)
{
+ WarnFailedToLoadAssembly(file, ex);
+ continue;
}
- }
- return folderAssembliesReferencingFakeItEasy
- .Concat(appDomainAssembliesReferencingFakeItEasy)
- .Concat(new[] { FakeItEasyAssembly })
- .Distinct();
+ yield return loadedAssembly;
+ }
}
- private static void WarnFailedToLoadAssembly(string assemblyFile, Exception e)
+ private static void WarnFailedToLoadAssembly(string path, Exception ex)
{
- var outputWriter = new DefaultOutputWriter(Console.Write);
- outputWriter.Write(
+ Write(
+ ex,
"Warning: FakeItEasy failed to load assembly '{0}' while scanning for extension points. Any IArgumentValueFormatters, IDummyDefinitions, and IFakeConfigurators in that assembly will not be available.",
- assemblyFile);
- outputWriter.WriteLine();
- using (outputWriter.Indent())
+ path);
+ }
+
+ private static void WarnFailedToGetTypes(Assembly assembly, Exception ex)
+ {
+ Write(
+ ex,
+ "Warning: FakeItEasy failed to get types from assembly '{0}' while scanning for extension points. Any IArgumentValueFormatters, IDummyDefinitions, and IFakeConfigurators in that assembly will not be available.",
+ assembly);
+ }
+
+ private static void Write(Exception ex, string messageFormat, params object[] messageArgs)
+ {
+ var writer = new DefaultOutputWriter(Console.Write);
+ writer.Write(messageFormat, messageArgs);
+ writer.WriteLine();
+ using (writer.Indent())
{
- outputWriter.Write(e.Message);
- outputWriter.WriteLine();
+ writer.Write(ex.Message);
+ writer.WriteLine();
}
}
}
View
2  Source/FakeItEasy/Creation/CastleDynamicProxy/CastleDynamicProxyGenerator.cs
@@ -157,7 +157,7 @@ private static object[] GetConstructorArgumentsArray(IEnumerable<object> argumen
private static IEnumerable<Type> GetAllInterfacesToImplement(IEnumerable<Type> additionalInterfacesToImplement)
{
- return additionalInterfacesToImplement.Concat(new[] { typeof(ITaggable) });
+ return additionalInterfacesToImplement.Concat(typeof(ITaggable));
}
[Serializable]
View
5 Source/FakeItEasy/EnumerableExtensions.cs
@@ -63,6 +63,11 @@ public static string ToCollectionString<T>(this IEnumerable<T> items, Func<T, st
return sequence.ToLookup(keySelector).ToDictionary(x => x.Key, x => x.First());
}
+ public static IEnumerable<T> Concat<T>(this IEnumerable<T> source, T item)
+ {
+ return source.Concat(new[] { item });
+ }
+
private class ZipEnumerable<TFirst, TSecond>
: IEnumerable<Tuple<TFirst, TSecond>>
{
View
6 Source/FakeItEasy/ImportsModule.cs
@@ -15,7 +15,11 @@ public override void RegisterDependencies(DictionaryContainer container)
container.RegisterSingleton<TypeCatalogueInstanceProvider>(c =>
new TypeCatalogueInstanceProvider(c.Resolve<ITypeCatalogue>()));
container.RegisterSingleton<ITypeCatalogue>(c =>
- new TypeCatalogue(bootstrapper.GetAssemblyFileNamesToScanForExtensions()));
+ {
+ var typeCatalogue = new TypeCatalogue();
+ typeCatalogue.Load(bootstrapper.GetAssemblyFileNamesToScanForExtensions());
+ return typeCatalogue;
+ });
RegisterEnumerableInstantiatedFromTypeCatalogue<IArgumentValueFormatter>(container);
RegisterEnumerableInstantiatedFromTypeCatalogue<IDummyDefinition>(container);
Something went wrong with that request. Please try again.