CI test coverage of F# unit tests

[abelbraaksma]

In some threads, specifically this one (#2745, on new Seq module) I was asked by @dsyme and @manofstick to have a look at test coverage.

I'm not 100% sure how to drive this, but let's get the good news out of the way first:

• I've managed to get NCrunch running with the NUnit (and FsCheck etc) tests (this wasn't trivial due to the way FSharp.sln is set up with dependencies that cannot be auto-detected from the prj file)
• Code coverage information is inserted in the editor, which gives immediate feedback over coverage
• Auto-impact analysis and run works (that is: change something in the source and only the impacted tests will be automatically run).

I'm not sure whether this is much different from other testing frameworks, but I hope it helps. Here's some idea of what to expect:

Coverage overview

Coverage per file

In-editor metrics/coverage of non-inline functions:

Red: line belongs to at least one failing test, green: all tests covering that line succeed, white: no tests cover that line or those lines have no debug information that can be used by metrics.

In-editor metrics/coverage of inline functions (not so good):

So, essentially what we are seeing here is:

• Tests seem to cover a large part of FSharp.Core, other projects are barely covered.
• Inline functions are not covered, this is possibly in part because line-timings do not work for some unclear reason (I have to research that, the error thrown says the assembly is invalid after metrics are added, perhaps to do with signature?)
  ** A bunch of functions seem to be inlined that shouldn't be or don't need to be, they can be disabled with a compile constant for testing / metrics / debugging only
  ** The missing inline coverage adds up significantly to the coverage reported, we can either exclude these lines or find another way of getting coverage (there are several techniques I use in my own projects)
• Execution line-timings coverage is not working, as per above (this would immediately show lines that run unusually long)

I hope this gives some insight. More needs to be done to make this report useful. I don't know if others in this community are using NCrunch (it's commercial), or that I can look into getting coverage with open source tools.

On my todo list (in no particular order and surely incomplete):

• Correct the coverage percentages w.r.t. inlining
• If a change to the code is accepted for this, I can look into fixing inline-coverage
• Perhaps add/report/document the necessary steps for setting this up
• Report issues with failing tests (some are failing because of internationalization issues, others are failing because of timeouts, I'll need to look into each of them)
• Discuss with the community how to drive this further
• Report back to NCrunch on the bugs found in coverage and line-timings

[manofstick]

Nice work :-)

This will definitely be handy for checking the new Seq implementation, except (outside of your control)...

Inline functions are not covered

I believe this is because the compiler throws away all line/line number information from where the function originated. I make this assertion not through detailed knowledge, but rather just from anecdotal evidence from when debugged, There are parts of my work code-base that have:

let 
#if !DEBUG
 inline
#endif
    private f ... =

to assist in just this case... (which isn't always possible with inline functions - i.e. ones that are using statically resolved generics...)

(But maybe someone with more intimate knowledge can confirm. If it is the case then maybe a task could be to put that metadata in (if possible...))

[forki]

Most thing in the compiler are tested indirectly by running compilation Am 14.09.2017 4:11 vorm. schrieb "Paul Westcott" <notifications@github.com>:

…

Nice work :-) This will definitely be handy for checking the new Seq implementation, except (outside of your control)... Inline functions are not covered I believe this is because the compiler throws away all line/line number information from where the function originated. I make this assertion not through detailed knowledge, but rather just from anecdotal evidence from when debugged, There are parts of my work code-base that have: let #if !DEBUG inline #endif private f ... = to assist in just this case... (which isn't always possible with inline functions - i.e. ones that are using statically resolved generics...) (But maybe someone with more intimate knowledge can confirm. If it is the case then maybe a task could be to put that metadata in (if possible...)) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3579 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNJLRvUuQxAn2Yyw8KN6qUExas7mOks5siItCgaJpZM4PW64x> .

[dsyme]

@abelbraaksma This is fantastic work. NUnit integration is likely enough for now, that will cover FSharp.Core.Unittests

• Could you write up step-by-step instructions, including which tools to install etc. and submti them to DEVGUIDE.md?
• Is it possible to get coverage information from CI runs?

thanks
don

[0x53A]

inline:

It should be possible to add sequence points that point to the original function in the IL.

I did a quick test, and it seems line inlined functions don't have any sequence points at all:

// Learn more about F# at http://fsharp.org
// See the 'F# Tutorial' project for more help.

let inline addAndMultiplyAndDivide a b c d =
    let x = a + b
    let y = x * c
    let z = y / d
    z

[<EntryPoint>]
let main argv = 
    let result = addAndMultiplyAndDivide 1. 2. 3. 4.
    printfn "%A" result
    printfn "%A" argv
    0 // return an integer exit code

// Token: 0x02000002 RID: 2
.class public auto ansi abstract sealed Program
	extends [mscorlib]System.Object
{
	.custom instance void [FSharp.Core]Microsoft.FSharp.Core.CompilationMappingAttribute::.ctor(valuetype [FSharp.Core]Microsoft.FSharp.Core.SourceConstructFlags) = (
		01 00 07 00 00 00 00 00
	)
	// Methods
	// Token: 0x06000001 RID: 1 RVA: 0x00002050 File Offset: 0x00000250
	.method public static 
		!!g addAndMultiplyAndDivide<a, b, c, d, e, f, g> (
			!!a a,
			!!b b,
			!!d c,
			!!f d
		) cil managed 
	{
		.custom instance void [FSharp.Core]Microsoft.FSharp.Core.CompilationArgumentCountsAttribute::.ctor(int32[]) = (
			01 00 04 00 00 00 01 00 00 00 01 00 00 00 01 00
			00 00 01 00 00 00 00 00
		)
		// Header Size: 12 bytes
		// Code Size: 62 (0x3E) bytes
		// LocalVarSig Token: 0x11000008 RID: 8
		.maxstack 4
		.locals init (
			[0] !!c x,
			[1] !!a,
			[2] !!b,
			[3] !!e y,
			[4] !!c,
			[5] !!d,
			[6] !!g z,
			[7] !!e,
			[8] !!f
		)

		/* (5,5)-(5,18) c:\users\lrieger\Source\Repos\ConsoleApplication2\ConsoleApplication2\Program.fs */
		/* 0x0000025C */ IL_0000: nop
		/* 0x0000025D */ IL_0001: ldarg.0
		/* 0x0000025E */ IL_0002: stloc.1
		/* 0x0000025F */ IL_0003: ldarg.1
		/* 0x00000260 */ IL_0004: stloc.2
		/* 0x00000261 */ IL_0005: ldloc.1
		/* 0x00000262 */ IL_0006: ldloc.2
		/* 0x00000263 */ IL_0007: call      !!2 [FSharp.Core]Microsoft.FSharp.Core.LanguagePrimitives::AdditionDynamic<!!a, !!b, !!c>(!!0, !!1)
		/* 0x00000268 */ IL_000C: stloc.0
		/* (6,5)-(6,18) c:\users\lrieger\Source\Repos\ConsoleApplication2\ConsoleApplication2\Program.fs */
		/* 0x00000269 */ IL_000D: ldloc.0
		/* 0x0000026A */ IL_000E: stloc.s   4
		/* 0x0000026C */ IL_0010: ldarg.2
		/* 0x0000026D */ IL_0011: stloc.s   5
		/* 0x0000026F */ IL_0013: ldloc.s   4
		/* 0x00000271 */ IL_0015: ldloc.s   5
		/* 0x00000273 */ IL_0017: call      !!2 [FSharp.Core]Microsoft.FSharp.Core.LanguagePrimitives::MultiplyDynamic<!!c, !!d, !!e>(!!0, !!1)
		/* 0x00000278 */ IL_001C: stloc.3
		/* (7,5)-(7,18) c:\users\lrieger\Source\Repos\ConsoleApplication2\ConsoleApplication2\Program.fs */
		/* 0x00000279 */ IL_001D: ldloc.3
		/* 0x0000027A */ IL_001E: stloc.s   7
		/* 0x0000027C */ IL_0020: ldarg.3
		/* 0x0000027D */ IL_0021: stloc.s   8
		/* 0x0000027F */ IL_0023: ldc.i4.0
		/* 0x00000280 */ IL_0024: brfalse.s IL_002E

		/* 0x00000282 */ IL_0026: ldnull
		/* 0x00000283 */ IL_0027: unbox.any !!g
		/* 0x00000288 */ IL_002C: br.s      IL_0039

		/* 0x0000028A */ IL_002E: ldstr     "Dynamic invocation of op_Division is not supported"
		/* 0x0000028F */ IL_0033: newobj    instance void [mscorlib]System.NotSupportedException::.ctor()
		/* 0x00000294 */ IL_0038: throw

		/* 0x00000295 */ IL_0039: stloc.s   z
		/* (8,5)-(8,6) c:\users\lrieger\Source\Repos\ConsoleApplication2\ConsoleApplication2\Program.fs */
		/* 0x00000297 */ IL_003B: ldloc.s   z
		/* 0x00000299 */ IL_003D: ret
	} // end of method Program::addAndMultiplyAndDivide

	// Token: 0x06000002 RID: 2 RVA: 0x0000209C File Offset: 0x0000029C
	.method public static 
		int32 main (
			string[] argv
		) cil managed 
	{
		.custom instance void [FSharp.Core]Microsoft.FSharp.Core.EntryPointAttribute::.ctor() = (
			01 00 00 00
		)
		// Header Size: 12 bytes
		// Code Size: 105 (0x69) bytes
		// LocalVarSig Token: 0x1100000D RID: 13
		.maxstack 4
		.entrypoint
		.locals init (
			[0] float64 result,
			[1] float64,
			[2] float64,
			[3] class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<float64, class [FSharp.Core]Microsoft.FSharp.Core.Unit>,
			[4] float64,
			[5] class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<string[], class [FSharp.Core]Microsoft.FSharp.Core.Unit>,
			[6] string[]
		)

		/* (12,5)-(12,53) c:\users\lrieger\Source\Repos\ConsoleApplication2\ConsoleApplication2\Program.fs */
		/* 0x000002A8 */ IL_0000: nop
		/* 0x000002A9 */ IL_0001: ldc.r8    1
		/* 0x000002B2 */ IL_000A: stloc.1
		/* 0x000002B3 */ IL_000B: ldc.r8    2
		/* 0x000002BC */ IL_0014: stloc.2
		/* 0x000002BD */ IL_0015: ldloc.1
		/* 0x000002BE */ IL_0016: ldloc.2
		/* 0x000002BF */ IL_0017: add
		/* 0x000002C0 */ IL_0018: ldc.r8    3
		/* 0x000002C9 */ IL_0021: mul
		/* 0x000002CA */ IL_0022: ldc.r8    4
		/* 0x000002D3 */ IL_002B: div
		/* 0x000002D4 */ IL_002C: stloc.0
		/* 0x000002D5 */ IL_002D: ldstr     "%A"
		/* 0x000002DA */ IL_0032: newobj    instance void class [FSharp.Core]Microsoft.FSharp.Core.PrintfFormat`5<class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<float64, class [FSharp.Core]Microsoft.FSharp.Core.Unit>, class [mscorlib]System.IO.TextWriter, class [FSharp.Core]Microsoft.FSharp.Core.Unit, class [FSharp.Core]Microsoft.FSharp.Core.Unit, float64>::.ctor(string)
		/* 0x000002DF */ IL_0037: call      !!0 [FSharp.Core]Microsoft.FSharp.Core.ExtraTopLevelOperators::PrintFormatLine<class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<float64, class [FSharp.Core]Microsoft.FSharp.Core.Unit>>(class [FSharp.Core]Microsoft.FSharp.Core.PrintfFormat`4<!!0, class [mscorlib]System.IO.TextWriter, class [FSharp.Core]Microsoft.FSharp.Core.Unit, class [FSharp.Core]Microsoft.FSharp.Core.Unit>)
		/* 0x000002E4 */ IL_003C: stloc.3
		/* (13,5)-(13,24) c:\users\lrieger\Source\Repos\ConsoleApplication2\ConsoleApplication2\Program.fs */
		/* 0x000002E5 */ IL_003D: ldloc.0
		/* 0x000002E6 */ IL_003E: stloc.s   4
		/* 0x000002E8 */ IL_0040: ldloc.3
		/* 0x000002E9 */ IL_0041: ldloc.s   4
		/* 0x000002EB */ IL_0043: callvirt  instance !1 class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<float64, class [FSharp.Core]Microsoft.FSharp.Core.Unit>::Invoke(!0)
		/* 0x000002F0 */ IL_0048: pop
		/* 0x000002F1 */ IL_0049: ldstr     "%A"
		/* 0x000002F6 */ IL_004E: newobj    instance void class [FSharp.Core]Microsoft.FSharp.Core.PrintfFormat`5<class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<string[], class [FSharp.Core]Microsoft.FSharp.Core.Unit>, class [mscorlib]System.IO.TextWriter, class [FSharp.Core]Microsoft.FSharp.Core.Unit, class [FSharp.Core]Microsoft.FSharp.Core.Unit, string[]>::.ctor(string)
		/* 0x000002FB */ IL_0053: call      !!0 [FSharp.Core]Microsoft.FSharp.Core.ExtraTopLevelOperators::PrintFormatLine<class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<string[], class [FSharp.Core]Microsoft.FSharp.Core.Unit>>(class [FSharp.Core]Microsoft.FSharp.Core.PrintfFormat`4<!!0, class [mscorlib]System.IO.TextWriter, class [FSharp.Core]Microsoft.FSharp.Core.Unit, class [FSharp.Core]Microsoft.FSharp.Core.Unit>)
		/* 0x00000300 */ IL_0058: stloc.s   5
		/* (14,5)-(14,22) c:\users\lrieger\Source\Repos\ConsoleApplication2\ConsoleApplication2\Program.fs */
		/* 0x00000302 */ IL_005A: ldarg.0
		/* 0x00000303 */ IL_005B: stloc.s   6
		/* 0x00000305 */ IL_005D: ldloc.s   5
		/* 0x00000307 */ IL_005F: ldloc.s   6
		/* 0x00000309 */ IL_0061: callvirt  instance !1 class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<string[], class [FSharp.Core]Microsoft.FSharp.Core.Unit>::Invoke(!0)
		/* 0x0000030E */ IL_0066: pop
		/* (15,5)-(15,6) c:\users\lrieger\Source\Repos\ConsoleApplication2\ConsoleApplication2\Program.fs */
		/* 0x0000030F */ IL_0067: ldc.i4.0
		/* 0x00000310 */ IL_0068: ret
	} // end of method Program::main

} // end of class Program

Note how the IL code in addAndMultiplyAndDivide does have sequence points, but the inlined one in main not.

This would probably also help with debugging, if the correct sequence points are emitted, it should be possible to debug an inlined function normally like a not-inlined one!

(the sequence points are the comments above the IL lines. Tool: dnSpy with

tool of choice:

I don't know if others in this community are using NCrunch (it's commercial)

I am using OpenCover + ReportGenerator.

It works with any test-runner, you just pass it any executable.

It does not embedd into the text editor, but it can generate nice html reports:

[dsyme]

This is correct, we don't generate any debug information for inlined functions. The specific places where we throw this information away are in the calls to copyExpr ...CloneAllAndMarkExprValsAsCompilerGenerated in Optimizer.fs

It's not something we would change lightly as it only seems to get noticed in code coverage reports - and maintaining the source marks might make for even more odd for step into and step over behaviour. There might be ways to emit exactly the right information to get correct behaviour for step into but I suspect not.

[0x53A]

as it only seems to get noticed in code coverage reports

I am pretty sure breakpoints in inlined functions also currently don't get hit.

[manofstick]

I am pretty sure breakpoints in inlined functions also currently don't get hit.

They don't. It's a pain.

[dsyme]

This is correct, we don't generate any debug information for inlined functions. The specific places where we throw this information away are in the calls to copyExpr ...CloneAllAndMarkExprValsAsCompilerGenerated in Optimizer.fs

It's not something we would change lightly - though the point about breakpoints is a good one.

I'm concerned that maintaining the source marks might make for even stranger step into and step over behaviour. Even more concerning is that maintaining the "locals" of the inlined function will mess with the display of locals for the calling function..

There might be ways to emit exactly the right information to get correct behaviour for these but I suspect not.

[abelbraaksma]

It's not something we would change lightly - though the point about breakpoints is a good one.

@dsyme, it's the breakpoints and stepping into, it's code coverage and it's profiling. I do a lot of profiling and the only way to find performance of an inline function is to write it as a non-inlined one, and often that's not even possible (think let inline add x y = x + y or explicit static member constraints), plus it may give the wrong timings. Line-level timings are useful and if we emit line-level info, profiling would benefit (as opposed to per-method timings).

I find myself trying very hard not to need debugging at all and do it the old JavaScript way with assertions, logging and the like, which adds a whole layer of complexity (you have to account for such methods not to be compiled at all in release builds). Debugging is not always required, but sometimes inevitable.

The real question to ask is, of course: what's the cost vs benefit? I think that even if the debugging experience is awkward but somewhat usable, it'd be already a big gain (in fact, with F#, a lot of the debugging experience is awkward anyway, so nobody would be surprised if debugging inlined functions isn't always consistent). We can improve with time.

And perhaps we could start with having debug builds not inlining methods when it is not required (i.e., let inline f x = Option.isSome x has the same semantics inlined and non-inlined, so a debug build can ditch the "inline"). This is a practice that some programmers (i.e., @manofstick, myself) already do by hand, but could be done automatically.

[abelbraaksma]

Btw, sometimes inline bindings or members do seem to get code coverage. I don't know what the rule of thumb is for when this happens, though. It seems random:

Or this (I assume the while does not get inlined here but ends up being its own non-inlined function, hence the coverage):

[dsyme]

Code coverage can be done using debug code. Standard recommendation is to only use explicit inline very rarely. If you require both code coverage and a lot of explcit inlining then I suppose you should currently use an #if as above.

Profiling is trickier. I don't think people expect accurate profilng when code is being inlined, nor will the CLR be able to provide it.

I think it would be reasonable for you to add an option to the compiler to maintain breakpoints and marks in inlined code.

Btw, sometimes inline bindings or members do seem to get code coverage. I don't know what the rule of thumb is for when this happens, though. It seems random:

Interesting! I think these will be missing cases in the erasure of marks from inlined code. It is also a good indication that adding an option to maintain sequence points and marks in inlined code will work well.

[abelbraaksma]

I think these will be missing cases in the erasure of marks from inlined code.

@dsyme, are you suggesting that this is deliberately erased? In that case, would it be possible to (perhaps temporarily on a separate branch or so) switch off this erase function and just "see what happens"? With a bit of luck it'll give us what we need (and we could turn it into a compiler directive/option or something). If not, at least we learn something ;).

[0x53A]

Code Coverage (and profiling) should also work reasonably well with Release code.

In the case of Profiling, debug code often is slow in different parts, so you may chaise the wrong issues.

We want to experiment with running all manual tests under Code Coverage. The reason is so that we can see which code paths will be used by real-world usage. And maybe extend the manual testing.

Manual testing is done with a Release build, because we want to test the bits that get shipped to the customer, not something possibly completely different.

[abelbraaksma]

Code Coverage (and profiling) should also work reasonably well with Release code.

@0x53A: yes, I see your point. Currently, coverage is much worse than with debug build, simply because much more gets inlined. Same with profiling, I agree that that (usually, not always) makes more sense in release builds.

But how would that work in practice? I can write 10 lines of code that end up being compiled into a single instruction three assemblies further, after passing through two other functions in two other assemblies (ok, not common, but still). So:

Assembly A defines inline method
Assembly B uses inline method, itself inlined
Assembly C uses inline method, non-inlined

You profile / coverage Assembly C. Does PDB (or where does it get stored?) support setting line numbers and source file directives to Assembly B from C and A from B and C?

While this is what one would expect, someone using Assembly C (without referencing Assembly A) would then potentially get stacktraces that involves Assembly A and/or B, right? Is that desirable?

I can imagine it isn't, which adds to the controversiality of such feature. I would welcome it, but I think we should not add it to Release builds, unless by compile-option, which could then be used for code-coverage and profiling.

[0x53A]

Does PDB (or where does it get stored?)

Yes.

support setting line numbers and source file directives to Assembly B from C and A from B and C

It is just a simple mapping IL instruction -> file path + start row/column + end row/column.

So yes, it would be possible to add a link to the original file in the pdb, but I don't think that makes sense, because you very likely don't have access to that file.

So in cross-assembly inlining, this should probably not be done.