andrewabest/Conventional

GetTypeDefinitionFor does not play nicely with runtime generated types (e.g. coverlet code coverage)

Opened this issue · 9 comments

flakey-bit commented

Apologies for the vague issue - we've been running into problems with conventions that use GetTypeDefinitionFor when types are emitted at runtime.

As an example, MustNotCallMethodConventionSpecification (and anything that dervies from it) is affected.

For us, the emitted types are coming from Coverlet.

Coverlet is a cross platform code coverage framework for .NET, with support for line, branch and method coverage

Basically, you add a package reference to coverlet.collector to your test projects only and then run tests with data collection on and it spits out a code-coverage report.

This code (which adds a module tracker) is responsible for emitting the types into the instrumented assembly.

When we run the tests normally (in the IDE or with dotnet test) everything is fine. It's when we run the tests with (Coverlet) data collection enabled

dotnet test --collect="XPlat Code Coverage"

that things break.

We see a test failure like this:

  Error Message:
   System.InvalidOperationException : Could not find type Coverlet.Core.Instrumentation.Tracker.MyCompany.MyApp.BFF.DTOs_cbb8ca5e-3732-4ccd-97be-dc8fcaf369f0 in C:\dev\MyCompany.MyApp.BFF\project\Tests\MyCompany.MyApp.BFF.ConventionTests\bin\Debug\net6.0\MyCompany.MyApp.BFF.DTOs.dll.
Types we can see:  - <Module>
 - Microsoft.CodeAnalysis.EmbeddedAttribute
 - System.Runtime.CompilerServices.NullableAttribute
 - System.Runtime.CompilerServices.NullableContextAttribute
 - {...various types}
 - Coverlet.Core.Instrumentation.Tracker.MyCompany.MyApp.BFF.DTOs_cbb8ca5e-3732-4ccd-97be-dc8fcaf369f0
  Stack Trace:
     at Conventional.Conventions.Cecil.DecompilationCache.<>c__DisplayClass10_0.<GetTypeDefinitionFor>b__0(Type t) in C:\dev\Conventional\src\Core\Conventional\Conventions\Cecil\DecompilationCache.cs:line 127
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at Conventional.Conventions.Cecil.DecompilationCache.GetTypeDefinitionFor(Type type) in C:\dev\Conventional\src\Core\Conventional\Conventions\Cecil\DecompilationCache.cs:line 112
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at Conventional.Conventions.Cecil.DecompilationCache.GetInstructionsFor(Type type) in C:\dev\Conventional\src\Core\Conventional\Conventions\Cecil\DecompilationCache.cs:line 48
   at Conventional.Conventions.Cecil.DecompilationCache.InstructionsFor(Type type, Boolean includeStateMachine) in C:\dev\Conventional\src\Core\Conventional\Conventions\Cecil\DecompilationCache.cs:line 31
   at Conventional.Conventions.Cecil.MustNotCallMethodConventionSpecification.IsSatisfiedBy(Type type) in C:\dev\Conventional\src\Core\Conventional\Conventions\Cecil\MustNotUseMethodSpecification.cs:line 33
   at System.Linq.Enumerable.WhereSelectArrayIterator`2.MoveNext()
   at System.Collections.Generic.List`1..ctor(IEnumerable`1 collection)
   at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)
   at Conventional.Conformist.WithFailureAssertion(IEnumerable`1 results, Action`1 failureAssertion) in C:\dev\Conventional\src\Core\Conventional\Conformist.cs:line 87
   at MyCompany.MyApp.BFF.ConventionTests.Code.CommonConventionTests.Convention_MustNotAccessNewRelicAgentStatically(Assembly assembly) in C:\dev\MyCompany.MyApp.BFF\project\Tests\MyCompany.MyApp.BFF.ConventionTests\Code\CommonConventionTests.cs:line 64

Note that the type it was looking for (Coverlet.Core.Instrumentation.Tracker.MyCompany.MyApp.BFF.DTOs_cbb8ca5e-3732-4ccd-97be-dc8fcaf369f0 in the example above) was in the list "Types we can see" (and is evidently in the module definition).

For whatever reason, ModuleDefinition::GetType() (Cecil) refuses to return the TypeDefinition.

We could work around it with something like moduleDefinition.Types.Single(mt => mt.FullName == type.FullName);, however that's missing the point - these generated types are an artefact of the tests running with code coverage on. Almost certainly, the fix is to exclude such types from convention checks.

An example of a convention test (theory) we had problems with:

  [Theory]
  [MemberData(nameof(EnumerateNonTestAssemblies))]
  public void Convention_MustNotAccessNewRelicAgentStatically(Assembly assembly)
  {
      // NewRelic agent (IAgent) should be injected rather than accessed statically via NewRelic.GetAgent()
      assembly.GetTypes()
          .Where(t => ((t.DeclaringType ?? t) != typeof(Startup)))
          .MustConformTo(new MustNotAccessNewRelicAgentStatically())
          .WithFailureAssertion(ConventionHelpers.Fail);
  }

(MustNotAccessNewRelicAgentStatically is based on MustNotCallMethodConventionSpecification).

We can work-around the problem by further filtering the types (from the assembly) e.g.

assembly.GetTypes()
    .Where(t => ((t.DeclaringType ?? t) != typeof(Startup)))
    .Where(type => type.FullName != null && !type.FullName.StartsWith("Coverlet"))

Wondering if this is the best approach or whether the Conventional library could help out with this in some way - thoughts?

andrewabest commented

G'day @flakey-bit - sorry about the very late response here, I missed this issue 😞. I'll have a read this afternoon and get back to you.

andrewabest commented

Okay, so, a fun, but complex problem. One tool uses Cecil to rewrite a module at runtime, and then another uses cecil to try and read module types at runtime.

I'm not entirely sure where the bridges aren't meeting here - as you've pointed out, the type is discoverable from the module definition, which means the first tool has modified the module, and we are reading the modified module.

I'm not sure if that is the problem to solve here though. We could either:

  • Treat the fact Conventional doesn't deal well with this particular class of runtime emitted types well as the issue, or
  • Treat the fact that Conventional doesn't give you an in-the-box way to deal with classes of types you wish to ignore for all tests.

If we look at the second one, there is an opportunity to register a set of types globally you don't want inspected for any tests.

These could be applied in https://github.com/andrewabest/Conventional/blob/master/src/Core/Conventional/Conformist.cs#L16-L35 to filter the set of types being inspected further.

With the additional filtering, you wouldn't need to remember to exclude these whenever you picked up a certain type of convention.

Would that work for you?

  • ❤️1
flakey-bit commented

Hey!

Thanks for looking into this.

If we look at the second one, there is an opportunity to register a set of types globally you don't want inspected for any tests.

With the additional filtering, you wouldn't need to remember to exclude these whenever you picked up a certain type of convention.

That certainly would be helpful 👍, however it would probably be more convenient if there was an overload that takes a type predicate (in particular, for the case of types that are generated at runtime).

You can always write the overload that takes a set of types to exclude in terms of the type-predicate overload.

flakey-bit commented

Let me know if I can help out in any way with this 🙂

andrewabest commented

Let me know if I can help out in any way with this 🙂

If you want to throw together a spike PR to help me understand your suggestion in a bit more detail we could take it from there.

flakey-bit commented

All I meant by a type predicate was a Func<Type, bool> i.e. something like this:

    public static class ConventionConfiguration
    {
        public static Action<string> DefaultFailureAssertionCallback { get; set; }
        // ...
        public static Func<Type, bool> DefaultTypeFilter { get; set; } = t => true;
    }

where all types are passed through DefaultTypeFilter.

I think it's just the one method of Conformist & AsyncConfirmist respectively that need to change:

public static WrappedConventionResult MustConformTo(this IEnumerable<Type> types, IConventionSpecification conventionSpecification)
{
    var filteredTypes = types.Where(ConventionConfiguration.DefaultTypeFilter).ToArray();
    return EnforceConformance(new WrappedConventionResult(
        filteredTypes,
        filteredTypes.Select(conventionSpecification.IsSatisfiedBy)));
}

&

public static async Task<WrappedConventionResult> MustConformTo(this IEnumerable<Type> types, IAsyncConventionSpecification conventionSpecification)
{
    var filteredTypes = types.Where(ConventionConfiguration.DefaultTypeFilter).ToArray();
    return Conformist.EnforceConformance(new WrappedConventionResult(
        filteredTypes,
        await Task.WhenAll(filteredTypes.Select(conventionSpecification.IsSatisfiedBy))));
}

I guess a consideration is that this effectively hides things from WrappedConventionResult. That might be fine, but some kind of "Skipped" outcome might be better? 🤔

andrewabest commented

I guess a consideration is that this effectively hides things from WrappedConventionResult. That might be fine, but some kind of "Skipped" outcome might be better?

I don't think that is necessary. We are just further filtering down the types under inspection. I understand what you are saying, but I think we could start without it, viewing it as a potential future optimisation if it bites anyone.

andrewabest commented

@flakey-bit did you want to take a look at the PR I've pushed up to see if it will address your issue?

flakey-bit commented

I've taken a look at the PR - looks good, thanks for putting that together!

It will address the issue 🙂