CI test coverage of F# unit tests #13018
Replies: 21 comments
-
|
Nice work :-) This will definitely be handy for checking the new Seq implementation, except (outside of your control)...
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: 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...)) |
Beta Was this translation helpful? Give feedback.
-
|
Most thing in the compiler are tested indirectly by running compilation
Am 14.09.2017 4:11 vorm. schrieb "Paul Westcott" <[email protected]>:
… 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>
.
|
Beta Was this translation helpful? Give feedback.
-
|
@abelbraaksma This is fantastic work. NUnit integration is likely enough for now, that will cover FSharp.Core.Unittests
thanks |
Beta Was this translation helpful? Give feedback.
-
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 codeNote how the IL code in 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 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:
|
Beta Was this translation helpful? Give feedback.
-
|
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 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 |
Beta Was this translation helpful? Give feedback.
-
I am pretty sure breakpoints in inlined functions also currently don't get hit. |
Beta Was this translation helpful? Give feedback.
-
They don't. It's a pain. |
Beta Was this translation helpful? Give feedback.
-
|
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 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 There might be ways to emit exactly the right information to get correct behaviour for these but I suspect not. |
Beta Was this translation helpful? Give feedback.
-
@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 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., |
Beta Was this translation helpful? Give feedback.
-
|
Btw, sometimes
Or this (I assume the
|
Beta Was this translation helpful? Give feedback.
-
|
Code coverage can be done using debug code. Standard recommendation is to only use explicit 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.
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. |
Beta Was this translation helpful? Give feedback.
-
@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 ;). |
Beta Was this translation helpful? Give feedback.
-
|
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. |
Beta Was this translation helpful? Give feedback.
-
@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 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. |
Beta Was this translation helpful? Give feedback.
-
Yes.
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. |
Beta Was this translation helpful? Give feedback.
-
Yes - not suggesting - it's a fact
Yes |
Beta Was this translation helpful? Give feedback.
-
|
You can see coverage of some inline functions if they're visited via an evaluated like in this StackOverflow answer, but that won't work for SRTP inlines, which results in a If the PDBs included inlined code, for code coverage in debug builds, the case where the caller is in another assembly (the unit test suite) is probably just as important as within-assembly inlining. |
Beta Was this translation helpful? Give feedback.
-
|
I'd like to be able to get coverage info (at least in debug builds) for functions that are inline for SRTP reasons (e.g. This example repo). remarkExpr m (copyExpr cenv.g CloneAllAndMarkExprValsAsCompilerGenerated expr)so I tried (clutching at straws) each of remarkExpr m (copyExpr cenv.g CloneAllAndMarkExprValsAsCompilerGenerated expr)
remarkExpr m (copyExpr cenv.g CloneAll expr)
(copyExpr cenv.g CloneAllAndMarkExprValsAsCompilerGenerated expr)
(copyExpr cenv.g CloneAll expr)
remarkExpr m exprbut none of those seemed to get me the ability to step into the inlined functions, or get coverage markers under NCrunch. |
Beta Was this translation helpful? Give feedback.
-
|
@marklam I'd love to get the answers to that, but since this thread started I've dealt with the impossibility to get coverage for inline. I've no idea if it can be fixed as simply as put by @dsyme above, let alone making it a compiler switch during debug builds. I also don't think that a workaround like in C# , which can set the line number, would help here, as I'd assume that also gets erased. Apart from code coverage, I think making it debuggable is an even harder challenge. |
Beta Was this translation helpful? Give feedback.
-
|
It would also (presumably) be useful to have that info in the pdb for release builds for the purposes of profiling. |
Beta Was this translation helpful? Give feedback.
-
|
I will track this as a feature request to add code coverage. @vzarytovskii FYI I would expect Codecov to be used, since that's used in lots of other .NET projects. |
Beta Was this translation helpful? Give feedback.
-
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'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:
** 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)
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):
Beta Was this translation helpful? Give feedback.
All reactions