search

SteveGilham / altcover

Cross-platform coverage gathering and processing tool set for dotnet/.Net Framework and Mono
MIT License
504 stars 18 forks source link

Working with Expecto Test Runner #11

Closed TheAngryByrd closed 6 years ago

TheAngryByrd commented 6 years ago

I'm just opening here as initial starting point. We can move to other repo's based on if they need PR's.

So I was trying to mimic coverlet's msbuild targets as much as I could like https://github.com/TheAngryByrd/ExpectoAltCover-Test/blob/master/tests/Directory.Build.targets. However when I go to run dotnet test I'm getting an issue with the Expecto Test adapter

An exception occurred while invoking executor 'executor://yolodev/expecto': Symbols were found but are not matching the assembly

This error seems to come from inside Mono.Cecil. https://github.com/jbevain/cecil/blob/master/Mono.Cecil/ModuleDefinition.cs#L1077

Repo: https://github.com/TheAngryByrd/ExpectoAltCover-Test

just run dotnet test in the tests folder to see the behavior.

Do you have any input as to what needs to change to make this work?

Thanks!

cc: @Alxandr @haf @AnthonyLloyd

haf commented 6 years ago

I think @mnie has been active with the .net core templates and might have an opinion on this.

I personally don't know, to be honest.

Alxandr commented 6 years ago

My (somewhat unqualified) guess is that this is not (really) related to expecto, nor the test runner, but coverlet doing something weird in post-build. To test this theory, I would take the test runner out of the equation (makes things much harder to test ironically). Can you try the following?

  1. Add an entrypoint to the test project that calls the standard expecto "run tests" method (if you don't already have one).
  2. Build the project, and run the MSBuild task that does the DLL rewriting.
  3. Manually run the rewritten test dll (using dotnet run).

See if it crashes and if you get more information. Unfortunately I won't be able to look too much at this for the next few days.

TheAngryByrd commented 6 years ago

I’ve tried it with dotnet run and it seems to work fine.

Alxandr commented 6 years ago

And you get instrumentation?

TheAngryByrd commented 6 years ago

Yes


<Target Name="MyTesting" DependsOnTargets="Restore;BuildProject">
      <!-- Altcover fails if _Reports folder doesn't exist -->
     <MakeDir  
            Directories="$(OutputPath)/netcoreapp2.0/_Reports"/>   
      <AltCover.Prepare 
        InputDirectory="$(OutputPath)/netcoreapp2.0"  
        SymbolDirectories="$(OutputPath)/netcoreapp2.0"  
        OutputDirectory="$(OutputPath)/netcoreapp2.0/__Saved$([System.DateTime]::UtcNow.ToString().Replace(':','-').Replace('/','-').Replace(' ','+'))" 
        XmlReport="$(OutputPath)/netcoreapp2.0/_Reports/MSBuildTest.xml"
        CallContext="@(CallContext)"
        />
      <Exec Command="dotnet run -f netcoreapp2.0 --no-build" ContinueOnError="WarnAndContinue"/>
      <AltCover.Collect 
        RecorderDirectory="$(OutputPath)/netcoreapp2.0"   
        LcovReport="./lcov.info"
        />  
  </Target>
dotnet msbuild /t:MyTesting
Microsoft (R) Build Engine version 15.6.82.30579 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

/usr/local/share/dotnet/sdk/2.1.101/Sdks/Microsoft.NET.Sdk/build/Microsoft.NET.Sdk.DefaultItems.targets(198,5): warning : A PackageReference for 'Microsoft.NETCore.App' was included in your project. This package is implicitly referenced by the .NET SDK and you do not typically need to reference it from your project. For more information, see https://aka.ms/sdkimplicitrefs [/Users/myuser/Documents/GitHub/altcoverIssues/expectoTestRunnerIssue/tests/tests.fsproj]
  Restore completed in 39.14 ms for /Users/myuser/Documents/GitHub/altcoverIssues/expectoTestRunnerIssue/tests/tests.fsproj.
  Restore completed in 17.43 ms for /Users/myuser/Documents/GitHub/altcoverIssues/expectoTestRunnerIssue/tests/tests.fsproj.
Build started, please wait...
  tests -> /Users/myuser/Documents/GitHub/altcoverIssues/expectoTestRunnerIssue/tests/bin/Debug/netcoreapp2.0/tests.dll
Build completed.

  Creating folder /Users/myuser/Documents/GitHub/altcoverIssues/expectoTestRunnerIssue/tests/bin/Debug/netcoreapp2.0/__Saved4-16-18+1-28-56+PM
  Saving files to /Users/myuser/Documents/GitHub/altcoverIssues/expectoTestRunnerIssue/tests/bin/Debug/netcoreapp2.0/__Saved4-16-18+1-28-56+PM
  Instrumenting files in /Users/myuser/Documents/GitHub/altcoverIssues/expectoTestRunnerIssue/tests/bin/Debug/netcoreapp2.0
     => /Users/myuser/Documents/GitHub/altcoverIssues/expectoTestRunnerIssue/tests/bin/Debug/netcoreapp2.0/__Saved4-16-18+1-28-56+PM/AltCover.Recorder.g.dll
     => /Users/myuser/Documents/GitHub/altcoverIssues/expectoTestRunnerIssue/tests/bin/Debug/netcoreapp2.0/__Saved4-16-18+1-28-56+PM/tests.dll

  Coverage Report: /Users/myuser/Documents/GitHub/altcoverIssues/expectoTestRunnerIssue/tests/bin/Debug/netcoreapp2.0/_Reports/MSBuildTest.xml

      /Users/myuser/Documents/GitHub/altcoverIssues/expectoTestRunnerIssue/tests/bin/Debug/netcoreapp2.0/tests.dll
                  <=  tests, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null
      /Users/myuser/Documents/GitHub/altcoverIssues/expectoTestRunnerIssue/tests/bin/Debug/netcoreapp2.0/AltCover.Recorder.g.dll
                  <=  AltCover.Recorder.g, Version=3.0.0.0, Culture=neutral, PublicKeyToken=c02b1a9f5b7cade8
  [09:28:58 INF] EXPECTO? Running tests... <Expecto>
  [09:28:58 INF] EXPECTO! 2 tests run in 00:00:00.0408560 for samples – 1 passed, 1 ignored, 0 failed, 0 errored. ᕙ໒( ˵ ಠ ╭͜ʖ╮ ಠೃ ˵ )७ᕗ <Expecto>
  ... /Users/myuser/Documents/GitHub/altcoverIssues/expectoTestRunnerIssue/tests/bin/Debug/netcoreapp2.0/_Reports/MSBuildTest.xml.0.acv
  ... /Users/myuser/Documents/GitHub/altcoverIssues/expectoTestRunnerIssue/tests/bin/Debug/netcoreapp2.0/_Reports/MSBuildTest.xml.1.acv
  8 visits recorded
  Coverage statistics flushing took 0.068 seconds
  Visited Classes 4 of 57 (7.02)
  Visited Methods 4 of 95 (4.21)
  Visited Points 5 of 217 (2.3)
  Visited Branches 0 of 145 (0)

  ==== Alternative Results (includes all methods including those without corresponding source) ====
  Alternative Visited Classes 4 of 59 (6.78)
  Alternative Visited Methods 4 of 122 (3.28)
SteveGilham commented 6 years ago

I recall at some point finding that Cecil was being unreliable in writing out symbols for the instrumented assemblies, but I can't remember where I left things -- I'll have to check through history to find the details. At the time, I didn't see a use-case for them, other than completeness.

SteveGilham commented 6 years ago

Relevant commentary from the code that I made as a note-to-self

    // Assembly with symbols pdb writing fails on .net core on Windows when writing with
    // System.NullReferenceException : Object reference not set to an instance of an object.
    // from deep inside Cecil -- but this works!!

where "this" is writing out symbols as .mdb; and 

    // Assembly with pdb writing fails on mono on Windows when writing with
    // System.NullReferenceException : Object reference not set to an instance of an object.
    // from deep inside Cecil
    // Pdb writing fails on mono on non-Windows with
    // System.DllNotFoundException : ole32.dll
    //  at (wrapper managed-to-native) Mono.Cecil.Pdb.SymWriter:CoCreateInstance
    // Mdb writing now fails in .net framework, it throws
    // Mono.CompilerServices.SymbolWriter.MonoSymbolFileException :
    // Exception of type 'Mono.CompilerServices.SymbolWriter.MonoSymbolFileException' was thrown.
    // If there are portable .pdbs on mono, those fail to write, too with
    // Mono.CompilerServices.SymbolWriter.MonoSymbolFileException :
    // Exception of type 'Mono.CompilerServices.SymbolWriter.MonoSymbolFileException' was thrown.

These comments were made in January, apart from the last bit about portable .pdbs on mono, at the start of March; which means I've not re-tested since Cecil 0.10 came out.

SteveGilham commented 6 years ago

I just made the experiment of setting the .net core build to write .pdb rather than .mdb, and Cecil 0.10 final still gives the same issue

 System.NullReferenceException : Object reference not set to an instance of an object.
Stack Trace:
   at Mono.Cecil.MetadataBuilder.AddLocalConstants(ScopeDebugInformation scope)
   at Mono.Cecil.MetadataBuilder.AddLocalScope(MethodDebugInformation method_info, ScopeDebugInformation scope)
   at Mono.Cecil.MetadataBuilder.AddMethodDebugInformation(MethodDebugInformation method_info)
   at Mono.Cecil.Cil.CodeWriter.WriteResolvedMethodBody(MethodDefinition method)
   at Mono.Cecil.Cil.CodeWriter.WriteMethodBody(MethodDefinition method)
   at Mono.Cecil.MetadataBuilder.AddMethod(MethodDefinition method)
   at Mono.Cecil.MetadataBuilder.AddMethods(TypeDefinition type)
   at Mono.Cecil.MetadataBuilder.AddType(TypeDefinition type)
   at Mono.Cecil.MetadataBuilder.AddNestedTypes(TypeDefinition type)
   at Mono.Cecil.MetadataBuilder.AddType(TypeDefinition type)
   at Mono.Cecil.MetadataBuilder.AddTypes()
   at Mono.Cecil.MetadataBuilder.BuildTypes()
   at Mono.Cecil.MetadataBuilder.BuildModule()
   at Mono.Cecil.MetadataBuilder.BuildMetadata()
   at Mono.Cecil.ModuleWriter.<>c.<BuildMetadata>b__2_0(MetadataBuilder builder, MetadataReader _)
   at Mono.Cecil.ModuleDefinition.Read[TItem,TRet](TItem item, Func`3 read)
   at Mono.Cecil.ModuleWriter.BuildMetadata(ModuleDefinition module, MetadataBuilder metadata)
   at Mono.Cecil.ModuleWriter.Write(ModuleDefinition module, Disposable`1 stream, WriterParameters parameters)
   at Mono.Cecil.ModuleWriter.WriteModule(ModuleDefinition module, Disposable`1 stream, WriterParameters parameters)
   at Mono.Cecil.ModuleDefinition.Write(String fileName, WriterParameters parameters)
   at AltCover.Instrument.WriteAssembly(AssemblyDefinition assembly, String path)
SteveGilham commented 6 years ago

Having had reason to look more deeply into this, it's starting to look like maybe this is another manifestation of this issue -- a bug in Cecil 0.10 beta6, that seems possibly fixed in the final build, but which gets masked by the unit test framework.

SteveGilham commented 6 years ago

Having moved the related tests to xUnit, it now works with pdb in => pdb out, mdb in => mdb out for the .net core build (full fat framework isn't yet touched). Prerelease build 430 has this change in.

TheAngryByrd commented 6 years ago

Seems to be working for me! Thanks!

Alxandr commented 6 years ago

So it works with XUnit, but not the Expecto runner?

SteveGilham commented 6 years ago

Any of the unit tests involving symbol output were being skewed in .net core by the fact that NUnit drags in Cecil too, and the latest release of that is still on 0.10 beta6, and we don't have AppDomain isolation to protect from different assemblies with the same version. At root, it's a Cecil versioning bug -- the assembly contracts changed, but the version has stayed the same.

By moving those tests to XUnit, I'm actually able to test what Cecil 0.10 final is doing; and I can once more (as was the state of play at the turn of the year) make AltCover write out .pdb files for .pdb in, and .mdb files for .mdb in and see it working.

SteveGilham commented 6 years ago

The functional change (in amongst all the test and build furniture) is this one.

SteveGilham commented 6 years ago

In release 3.0.433