Proper test and resolution for the issue

SteveGilham · Sep 13, 2022 · 82098d9 · 82098d9
1 parent 5834548
commit 82098d9
Show file tree

Hide file tree

Showing 6 changed files with 45 additions and 15 deletions.
diff --git a/AltCover.Engine/CecilEx.fs b/AltCover.Engine/CecilEx.fs
@@ -217,11 +217,11 @@ module internal CecilExtension =
                                                                | 0x1euy // ElementType.MVar
                                                                | 0x0euy // ElementType.String
                                                                    -> false
-                                                               | _ -> true)
+                                                               | _ -> not c.ConstantType.IsPrimitive)
                                       |> Seq.toList
                              sus |> Seq.iter (fun c -> scope.Constants.Remove c |> ignore
-                                                       sprintf "Null Primitive Constant %s in method %s" c.Name m.FullName
-                                                       |> Output.warn))
+                                                       sprintf "Null Constant %s elided in method %s" c.Name m.FullName
+                                                       |> Output.verbose))
 
   let prepareLocalScopes (m: MethodDefinition) =
     offsetTable.Clear()

diff --git a/AltCover.Tests/Expecto.fs b/AltCover.Tests/Expecto.fs
@@ -519,6 +519,7 @@ module ExpectoTestManifest =
        "Tests2.ShouldGetTrackingStyleIfSet"
        Tests.AltCoverTests2.ShouldGetNewFilePathFromPreparedAssembly,
        "Tests2.ShouldGetNewFilePathFromPreparedAssembly"
+       Tests.AltCoverTests2.ShouldHandleNullConstantsOK, "Tests2.ShouldHandleNullConstantsOK"
        Tests.AltCoverTests2.ShouldRescopeMonoMethodOK, "Tests2.ShouldRescopeMonoMethodOK"
        Tests.AltCoverTests2.ShouldWriteMonoAssemblyOK, "Tests2.ShouldWriteMonoAssemblyOK"
        Tests.AltCoverTests2.ShouldGetVisitFromWrittenAssembly,

diff --git a/AltCover.Tests/NullConst.dll b/AltCover.Tests/NullConst.dll
diff --git a/AltCover.Tests/NullConst.pdb b/AltCover.Tests/NullConst.pdb
diff --git a/AltCover.Tests/Tests2.fs b/AltCover.Tests/Tests2.fs
@@ -833,11 +833,45 @@ module AltCoverTests2 =
     finally
       CoverageParameters.keys.Clear()
 
+  [<Test>]
+  let ShouldHandleNullConstantsOK () =
+    // Workround for Cecil 11.4
+    let path = // Use a known good (bad) build rather than a local new one each time
+      Path.Combine(SolutionRoot.location, "AltCover.Tests/NullConst.dll")
+    use ``module`` =
+      AssemblyResolver.ReadAssembly path
+
+    ProgramDatabase.readSymbols ``module``
+
+    // big test -- if we can write w/o crashing when the rescope is removed
+    let output = Path.GetTempFileName()
+    let outputdll = output + ".dll"
+
+    let writer = WriterParameters()
+    writer.SymbolWriterProvider <- Mono.Cecil.Cil.EmbeddedPortablePdbWriterProvider()
+    writer.WriteSymbols <- true
+
+    let save = Output.verbose
+    let b = System.Text.StringBuilder()
+
+    Output.verbose <- b.AppendLine >> ignore
+
+    let def =
+      ``module``.MainModule.GetTypes()
+      |> Seq.collect (fun t -> t.Methods)
+      |> Seq.find (fun m -> m.Name.Equals("MakeConst"))
+    prepareLocalScopes def
+    Output.verbose <- save
+    test <@ b.ToString() = "Null Constant thing elided in method System.Void NullConst.Program::MakeConst()" + Environment.NewLine @>
+
+    use sink =
+      File.Open(outputdll, FileMode.Create, FileAccess.ReadWrite)
+    ``module``.Write(sink, writer)
+
   [<Test>]
   let ShouldRescopeMonoMethodOK () =
     // Workround for Cecil 11.4
     let path = // Use a known good (bad) build rather than a local new one each time
-      // Path.Combine(SolutionRoot.location, "_Mono/Sample31/Sample31.dll")
       Path.Combine(SolutionRoot.location, "AltCover.Tests/Sample31.dll")
 
     use ``module`` =
@@ -878,7 +912,7 @@ module AltCoverTests2 =
 
     let writer = WriterParameters()
     writer.SymbolWriterProvider <- Mono.Cecil.Cil.EmbeddedPortablePdbWriterProvider()
-    //Mono.Cecil.Mdb.MdbWriterProvider()
+
     writer.WriteSymbols <- true
 
     use sink =
@@ -929,8 +963,6 @@ has been prefixed with Ldc_I4_1 (1 byte)
 
     pathGetterDef.DebugInformation.Scope <- rescope
 
-    let bexpect = System.Text.StringBuilder()
-
     [ -1
       4
       finish.Offset
@@ -944,18 +976,14 @@ has been prefixed with Ldc_I4_1 (1 byte)
       let n = i.ToString().Replace("-", "_")
       s.Constants.Add <| ConstantDebugInformation("I"+n, primitive, null)
       s.Constants.Add <| ConstantDebugInformation("A"+n, np, null)
-
-      sprintf "Null Primitive Constant I%s in method System.Boolean Sample31.Class3/Class4::get_Defer()" n
-      |> bexpect.AppendLine
-      |> ignore
       rescope.Scopes.Add s)
 
     rescope.Scopes.Add rescope
 
-    let save = Output.warn
+    let save = Output.verbose
     let b = System.Text.StringBuilder()
 
-    Output.warn <- b.AppendLine >> ignore
+    Output.verbose <- b.AppendLine >> ignore
 
     prepareLocalScopes pathGetterDef
 
@@ -984,9 +1012,9 @@ has been prefixed with Ldc_I4_1 (1 byte)
       |> Seq.toList
 
     test <@ result = expected @>
-    test <@ b.ToString() = bexpect.ToString() @>
+    test <@ b.ToString() = String.Empty @>
 
-    Output.warn <- save
+    Output.verbose <- save
 
   [<Test>]
   let ShouldWriteMonoAssemblyOK () =

diff --git a/ReleaseNotes.md b/ReleaseNotes.md
@@ -4,6 +4,7 @@ A. Start with the Quick Start guide : https://github.com/SteveGilham/altcover/wi
 
 # 8.3.8xx (Habu series release 15)
 * [BUGFIX] -- Issue #161: Add retry logic to `ContingentCopy` task 
+* [BUGFIX] -- Issue #165: Work around Cecil issue #873 by omitting debug information for constants that cause that issue. **N.B.** will not catch cases in methods that are not instrumented.
 
 # 8.3.838 (Habu series release 14)
 * [GTK-VISUALIZER] Fix off-by-one error in markup for the very last line of the file