GNOME Bugzilla – Bug 788166
Fix code coverage (and refactor it to take advantage of mozjs52 features)
Last modified: 2017-10-30 01:53:02 UTC
The code coverage report is currently broken because of errors reading files that previously were not reported. Also, it's not analyzing methods in ES6 classes.
Created attachment 360398 [details] [review] coverage: Don't error out on various files The coverage statistics should handle files with #! on the first line, and ignore files with names within angle brackets, which is our convention for scripts that are fed to the JS engine programmatically.
Created attachment 360399 [details] [review] coverage: Scan ES6 classes for functions Previously, class methods and property getters/setters were not counted for coverage statistics.
Created attachment 360400 [details] [review] coverage: Correct AST walking for labelled statements The AST string constant for labelled statements uses the alternate spelling 'LabeledStatement', so these were not getting scanned correctly.
Created attachment 360401 [details] [review] coverage: Walk AST in more cases - The switch expression of a switch-case statement - Default parameters to a function
Created attachment 360402 [details] [review] tests: Create test ASTs by parsing JS The previous approach of object literals as ASTs is not robust when SpiderMonkey adds to or changes the AST structure that its parser returns.
Created attachment 360403 [details] [review] coverage: Don't count literals as executable The AST type used to detect this is "Literal", previously it was given as "LiteralExpression" which caused literals on their own line to be counted as executable, but never executed during code coverage measurements. Requires some changes in the tests, since there was some test data assuming that literals were executable.
Created attachment 360404 [details] [review] coverage: Don't mark empty var declarations executable These never get executed. I'm not sure why they are treated differently than empty let declarations, but it's probably to do with scope.
Created attachment 360405 [details] [review] coverage: Refactor bootstrap code to use ES6 classes This makes the code clearer because it's less dependent on variables being captured in closures. (Although there are still a few places where that is necessary in the debugger event handlers.)
Created attachment 360406 [details] [review] coverage: Use frame callee script data to get line Previously, the coverage code had to deal with the entry point of a function (and therefore a frame) being on a different line than the definition of the function. So, when entering a debugger frame, we had to walk backwards through the source until a matching function definition was encountered. This was slow and complicated. Now, we can get the line where the function was defined by accessing the Debugger.Script object belonging to the frame's callee. This allows us to get rid of the walking-backwards code, and stop keeping an array of lines with known functions. Unfortunately even though the Reflect.parse API now gives column information, you still can't get the column where a function was defined from the Debugger.Script object. So we still have to disambiguate functions defined on the same line using their number of arguments.
Created attachment 360407 [details] [review] coverage: Use Map objects for function counters We can hopefully make the function counters more efficient by using Map objects instead of plain objects, and since we now always have accurate info about the line where a function is defined, we can combine the function's name and line into one key, thereby only maintaining two levels of Map instead of three. This also refers to a function map key (name:line:nArgs) as a 'key' everywhere, and not a 'name'.
Created attachment 360408 [details] [review] coverage: Misc refactors - Include default code coverage options in makefile so that branch coverage works when enabled - Fix unused parameter in C function - Remove test for deprecated SpiderMonkey syntax (it still works, we're just not going to test it)
Sam, would you be interested in reviewing this?
Review of attachment 360398 [details] [review]: Looks good
Review of attachment 360399 [details] [review]: Looks good too
Review of attachment 360400 [details] [review]: Wow, I didn't even know this was part of the spec. Thanks for fixing.
Review of attachment 360401 [details] [review]: Thanks for catching this
Review of attachment 360402 [details] [review]: Yeah, fair enough - though surprising that the AST definition is not stable.
Review of attachment 360403 [details] [review]: Cross-checked with https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Parser_API#Reflect.parse(src_options) and this makes sense - I assume that we are testing for both literals and literal expressions because the functions all evaluate "1, 2 + 3, 4 + 5" etc
Review of attachment 360404 [details] [review]: Also surprising, thanks for catching. As a follow-up, what happens when you have an empty `const` expression?
Review of attachment 360405 [details] [review]: ::: modules/_bootstrap/coverage.js @@ +679,3 @@ } +var _BranchTracker = class { `var _BranchTracker = class {` here seems strange, is there any reason why we can't have `class _BranchTracker` ? @@ +688,2 @@ /* Set branch exits or find a new active branch */ + if (this._activeBranch) { Previously we explicitly checked for `undefined` - I'm not entirely sure why we did that since we use `activeBranch` below, but can you confirm that we aren't setting it to `null` anywhere and behaving differently based on that? @@ +793,3 @@ } +var CoverageStatisticsContainer = class { See above, using `class CoverageStatisticsContainer` would be nicer @@ +806,3 @@ + let counters = _fetchCountersFromCache(filename, this._cachedASTs, + nLines); This indentation looks a bit odd @@ +929,3 @@ + if (frame.callee !== null && frame.callee.callable) { + let name = frame.callee.name || '(anonymous)'; + let {lineNumber} = frame.script.getOffsetLocation(frame.offset); The convention here is to put a space between the two brackets: let { lineNumber } = frame.script.getOffsetLine(frame.offset); @@ +934,3 @@ + try { + _incrementFunctionCounters(statistics.functionCounters, + statistics.linesWithKnownFunctions, name, lineNumber, This indentation looks a bit odd @@ +951,3 @@ + /* Line counts */ + let {offset} = this; + let {lineNumber} = this.script.getOffsetLocation(offset); See above on spacing convention
Review of attachment 360406 [details] [review]: Nice find! I remember back when I wrote this that it felt like an evil hack. Only a minor style nit. ::: modules/_bootstrap/coverage.js @@ -930,2 +876,4 @@ let name = frame.callee.name || '(anonymous)'; - let {lineNumber} = frame.script.getOffsetLocation(frame.offset); + // FIXME: https://bugzilla.mozilla.org/show_bug.cgi?id=901138 + // There should be a Debugger.Script.prototype.startColumn + let {startLine} = frame.script; `let { startLine }`
Review of attachment 360407 [details] [review]: Yeah, this should hopefully make things a little faster - I believe Map key lookups are O(log n) too, which is nice. ::: installed-tests/js/testCoverage.js @@ +1143,3 @@ /* Functions start on line 1 */ + expect([...statisticsWithNoCaching.functionCounters.keys()]) + .toEqual([...statistics.functionCounters.keys()]); It looks like this should be fine since Map keys are iterated through in insertion order (like object keys) https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map ::: modules/_bootstrap/coverage.js @@ +504,3 @@ functions.forEach(function(func) { + let [name] = func.key.split(':', 1); + let {line, n_params} = func; Space before brackets. @@ +540,3 @@ * we satisfy the number of arguments exactly, or failing that, * go through each and pick the closest. */ + let allNArgsOptions = [...argsMap.keys()]; We were calling parseInt beforehand here, are the keys here always Number-typed now? @@ +663,3 @@ + let [name, line] = nameLineKey.split(':'); + line = Number(line); + for (let [nArgs, {hitCount}] of linePart) { Space between brackets @@ +776,3 @@ branches: [], + functions: _convertFunctionCountersToArray(statisticsForFilename.functionCounters) + .map(({key, line, nArgs}) => ({key, line, n_params: nArgs})), Space between brackets
Review of attachment 360408 [details] [review]: ::: gjs/coverage.cpp @@ +442,3 @@ + !inserter(c_side_array, context, element)) { + g_array_unref(c_side_array); + return false; Should we still throw here?
Thanks for looking at all this! I know the coverage stuff is frustrating to maintain because its so dependent on the SM API. Only some minor comments and questions on my side.
Thanks for the quick reviews, Sam! Here are some replies, though on some things I'll have to get back to you later. Incidentally, have you seen Debugger.Script.getOffsetsCoverage()? (documentation at https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Debugger.Script, scroll down a bit) I think it must have not existed when you first wrote this, but it could potentially make things a lot easier. I think it can replace parsing the ASTs for executable lines and functions, though I'm not sure how it would work with the branch coverage. (In reply to Sam Spilsbury from comment #17) > Review of attachment 360402 [details] [review] [review]: > > Yeah, fair enough - though surprising that the AST definition is not stable. Every so often they do tend to change it up, I ran into that a few times while upgrading SpiderMonkey versions. I guess because Reflect.parse isn't actually part of the language spec, and their parser changes so much? (In reply to Sam Spilsbury from comment #19) > Review of attachment 360404 [details] [review] [review]: > > Also surprising, thanks for catching. As a follow-up, what happens when you > have an empty `const` expression? A syntax error. That makes sense to me, since you could never assign it a value later. (In reply to Sam Spilsbury from comment #20) > Review of attachment 360405 [details] [review] [review]: > > ::: modules/_bootstrap/coverage.js > @@ +679,3 @@ > } > > +var _BranchTracker = class { > > `var _BranchTracker = class {` here seems strange, is there any reason why > we can't have `class _BranchTracker` ? `class _BranchTracker` has the same scope semantics as `let foo`; it won't be exported on the module object and so won't be accessible outside the module according to ES6 scope rules. > @@ +688,2 @@ > /* Set branch exits or find a new active branch */ > + if (this._activeBranch) { > > Previously we explicitly checked for `undefined` - I'm not entirely sure why > we did that since we use `activeBranch` below, but can you confirm that we > aren't setting it to `null` anywhere and behaving differently based on that? Will get back to you on this. > + let counters = _fetchCountersFromCache(filename, this._cachedASTs, > + nLines); > > This indentation looks a bit odd I'm trying to standardize all the new JS code in GJS on one style. 4-space indents, rather than indenting to the first bracket, seemed to be more common. (Eventually I need to write all this up in a style guide.) > @@ +929,3 @@ > + if (frame.callee !== null && frame.callee.callable) { > + let name = frame.callee.name || '(anonymous)'; > + let {lineNumber} = > frame.script.getOffsetLocation(frame.offset); > > The convention here is to put a space between the two brackets: > > let { lineNumber } = frame.script.getOffsetLine(frame.offset); The JS community seems to be moving away from that style, since it's a bit cumbersome with object destructuring. So, like the indents, I'm trying to standardize new code on not using it. (In reply to Sam Spilsbury from comment #22) > Review of attachment 360407 [details] [review] [review]: > @@ +540,3 @@ > * we satisfy the number of arguments exactly, or failing that, > * go through each and pick the closest. */ > + let allNArgsOptions = [...argsMap.keys()]; > > We were calling parseInt beforehand here, are the keys here always > Number-typed now? They should be, in any case I deliberately changed that. (In reply to Sam Spilsbury from comment #23) > Review of attachment 360408 [details] [review] [review]: > > ::: gjs/coverage.cpp > @@ +442,3 @@ > + !inserter(c_side_array, context, element)) { > + g_array_unref(c_side_array); > + return false; > > Should we still throw here? No - I noticed while debugging that the second exception gets discarded anyway since there is already an exception pending when inserter() fails.
Attachment 360398 [details] pushed as 2a1ed02 - coverage: Don't error out on various files Attachment 360399 [details] pushed as 547a8a4 - coverage: Scan ES6 classes for functions Attachment 360400 [details] pushed as b1e860a - coverage: Correct AST walking for labelled statements Attachment 360401 [details] pushed as 92b0792 - coverage: Walk AST in more cases Attachment 360402 [details] pushed as 0c4ca17 - tests: Create test ASTs by parsing JS Attachment 360403 [details] pushed as 1fba725 - coverage: Don't count literals as executable Attachment 360404 [details] pushed as 023bf89 - coverage: Don't mark empty var declarations executable
(In reply to Philip Chimento from comment #25) > (In reply to Sam Spilsbury from comment #20) > > Review of attachment 360405 [details] [review] [review] [review]: > > @@ +688,2 @@ > > /* Set branch exits or find a new active branch */ > > + if (this._activeBranch) { > > > > Previously we explicitly checked for `undefined` - I'm not entirely sure why > > we did that since we use `activeBranch` below, but can you confirm that we > > aren't setting it to `null` anywhere and behaving differently based on that? > > Will get back to you on this. I'm pretty sure that only an array of objects can be returned from _branchesToBranchCounters(), and the objects are what end up in this._activeBranch. Let me know if you have any comments on my comments; I think the only thing left where we differ is the coding style.
(In reply to Philip Chimento from comment #25) > Thanks for the quick reviews, Sam! Here are some replies, though on some > things I'll have to get back to you later. > > Incidentally, have you seen Debugger.Script.getOffsetsCoverage()? > (documentation at > https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Debugger.Script, > scroll down a bit) I think it must have not existed when you first wrote > this, but it could potentially make things a lot easier. I think it can > replace parsing the ASTs for executable lines and functions, though I'm not > sure how it would work with the branch coverage. > > (In reply to Sam Spilsbury from comment #17) > > Review of attachment 360402 [details] [review] [review] [review]: > > > > Yeah, fair enough - though surprising that the AST definition is not stable. > > Every so often they do tend to change it up, I ran into that a few times > while upgrading SpiderMonkey versions. I guess because Reflect.parse isn't > actually part of the language spec, and their parser changes so much? > > (In reply to Sam Spilsbury from comment #19) > > Review of attachment 360404 [details] [review] [review] [review]: > > > > Also surprising, thanks for catching. As a follow-up, what happens when you > > have an empty `const` expression? > > A syntax error. That makes sense to me, since you could never assign it a > value later. > > (In reply to Sam Spilsbury from comment #20) > > Review of attachment 360405 [details] [review] [review] [review]: > > > > ::: modules/_bootstrap/coverage.js > > @@ +679,3 @@ > > } > > > > +var _BranchTracker = class { > > > > `var _BranchTracker = class {` here seems strange, is there any reason why > > we can't have `class _BranchTracker` ? > > `class _BranchTracker` has the same scope semantics as `let foo`; it won't > be exported on the module object and so won't be accessible outside the > module according to ES6 scope rules. Ah, that's a shame. I imagine its not available even if you use `export class` either, isn't it? > > > @@ +688,2 @@ > > /* Set branch exits or find a new active branch */ > > + if (this._activeBranch) { > > > > Previously we explicitly checked for `undefined` - I'm not entirely sure why > > we did that since we use `activeBranch` below, but can you confirm that we > > aren't setting it to `null` anywhere and behaving differently based on that? > > Will get back to you on this. > > > + let counters = _fetchCountersFromCache(filename, this._cachedASTs, > > + nLines); > > > > This indentation looks a bit odd > > I'm trying to standardize all the new JS code in GJS on one style. 4-space > indents, rather than indenting to the first bracket, seemed to be more > common. (Eventually I need to write all this up in a style guide.) Fair enough. Personally I don't like that style, but consistency trumps here. > > > @@ +929,3 @@ > > + if (frame.callee !== null && frame.callee.callable) { > > + let name = frame.callee.name || '(anonymous)'; > > + let {lineNumber} = > > frame.script.getOffsetLocation(frame.offset); > > > > The convention here is to put a space between the two brackets: > > > > let { lineNumber } = frame.script.getOffsetLine(frame.offset); > > The JS community seems to be moving away from that style, since it's a bit > cumbersome with object destructuring. So, like the indents, I'm trying to > standardize new code on not using it. Interesting, well, its probably good to standardise on something - happy to standardise on not using spaces. > > (In reply to Sam Spilsbury from comment #22) > > Review of attachment 360407 [details] [review] [review] [review]: > > @@ +540,3 @@ > > * we satisfy the number of arguments exactly, or failing that, > > * go through each and pick the closest. */ > > + let allNArgsOptions = [...argsMap.keys()]; > > > > We were calling parseInt beforehand here, are the keys here always > > Number-typed now? > > They should be, in any case I deliberately changed that. Makes sense. > > (In reply to Sam Spilsbury from comment #23) > > Review of attachment 360408 [details] [review] [review] [review]: > > > > ::: gjs/coverage.cpp > > @@ +442,3 @@ > > + !inserter(c_side_array, context, element)) { > > + g_array_unref(c_side_array); > > + return false; > > > > Should we still throw here? > > No - I noticed while debugging that the second exception gets discarded > anyway since there is already an exception pending when inserter() fails. Makes sense.
Review of attachment 360405 [details] [review]: OK after last discussion
Review of attachment 360406 [details] [review]: OK after last discussion
Review of attachment 360407 [details] [review]: OK after last discussion
Review of attachment 360408 [details] [review]: OK after last discussion
(In reply to Sam Spilsbury from comment #28) > (In reply to Philip Chimento from comment #25) > > (In reply to Sam Spilsbury from comment #20) > > > Review of attachment 360405 [details] [review] [review] [review] [review]: > > > > > > ::: modules/_bootstrap/coverage.js > > > @@ +679,3 @@ > > > } > > > > > > +var _BranchTracker = class { > > > > > > `var _BranchTracker = class {` here seems strange, is there any reason why > > > we can't have `class _BranchTracker` ? > > > > `class _BranchTracker` has the same scope semantics as `let foo`; it won't > > be exported on the module object and so won't be accessible outside the > > module according to ES6 scope rules. > > Ah, that's a shame. I imagine its not available even if you use `export > class` either, isn't it? `export class` should work, but it's not supported yet by SpiderMonkey without a transpiler. That said, I'm going to hold off on committing these patches for now. I've been checking out Debugger.Script.getOffsetsCoverage() and it is fantastic. It really cuts down on a lot of this code, since some of our stuff here is now also implemented inside SpiderMonkey. I'll post a WIP patch tonight after work so you can see what you think of it, but it will make most of these remaining refactors obsolete :-)
Created attachment 360572 [details] [review] coverage: Refactor to use Debugger.Script.getOffsetsCoverage()
Comment on attachment 360572 [details] [review] coverage: Refactor to use Debugger.Script.getOffsetsCoverage() Check this out Sam! Obviously not ready for review yet but it looks promising, no?
Looks promising indeed. From looking at the documentation, one of the big benefits that I see is that collectCoverageInfo actually recompiles all scripts to insert instrumentation, which means we don't have to use the old breakpoint-and-continue hack. You might also be able to remove all the complication around caching too - generating those ASTs and executable lines was super expensive, hence the complicated caching logic.
I don't think we'll be able to remove it entirely. As far as I can tell, there's no way to get branch tracking information from collectCoverageInfo, so we still need to analyze the AST to identify branches. The code that I have so far still steps through each frame to see which branches were taken, although maybe that's not necessary. In any case, running the tests with coverage enabled is a lot faster now, even though we still parse each file and walk the AST once for branch info. So maybe removing the caching is fine, I don't know. In other news, I found that passing a "prefixes" array to GjsCoverage is currently a no-op :-( Do you have an opinion on whether that should be fixed or removed?
Comment on attachment 360405 [details] [review] coverage: Refactor bootstrap code to use ES6 classes Attachment 360405 [details] pushed as 65e0e14 - coverage: Refactor bootstrap code to use ES6 classes
Created attachment 360643 [details] [review] coverage: Refactor to use Debugger.Script.getOffsetsCoverage() SpiderMonkey's debugger can keep track of coverage information internally. Taking advantage of this feature allows us to get more accurate coverage data and remove a lot of our coverage counters code. For line coverage we request a map of line/column positions of all opcodes in a script (getAllOffsets()), which tells us which lines are executable. Then we use getOffsetsCoverage() to discover which of those lines were actually executed. (A "script" in the SpiderMonkey debugger corresponds to either a top-level file, or a function.) For function counters, we count the number of times the script's first opcode was executed. Running with code coverage does become a lot faster. We also get access to the JS engine's internal naming convention for functions, so we don't have the problem of being unable to distinguish two functions' names. We now only have to cache the branch coverage info, so the cache becomes smaller too. We can remove the previous implementation of expression counters and function counters, since the same info is available from SpiderMonkey. We do still need to keep track of branch counters, so for now we still have to parse each JS script and analyze its AST. This implementation has a few quirks: if SpiderMonkey mistakenly marks a line executable or executed then we have less recourse to change it. For example, in code such as the following: if (room_on_fire) throw new Error('This is fine'); The throw statement will be marked as executed even if it wasn't. This is presumably a bug in SpiderMonkey that can be fixed.
Created attachment 360644 [details] [review] coverage: Misc refactors - Enable branch coverage by default, and include default code coverage options in makefile so that branch coverage works - Eliminate some double exceptions being thrown in cache code - Fix unused parameter in C++ function - Remove unused constant dealing with binary cache which we don't have anymore - Tighten assertions in some tests - Remove or change some obsolete comments
Created attachment 360689 [details] [review] coverage: Stop using cache Now that we don't have to do quite so much analysis of JS scripts' ASTs, it's quite feasible to do the whole coverage thing without caching. A test run with coverage doesn't take very much longer than a test run without.
Created attachment 360692 [details] [review] coverage: Fix coverage prefixes Filename prefixes passed to gjs_coverage_new() were ignored. This implements the expected functionality. Coverage is only recorded for JS files which are prefixed with one of the specified prefixes.
Review of attachment 360643 [details] [review]: ::: modules/_bootstrap/coverage.js @@ +328,3 @@ + if (!coverage) + return; + coverage.forEach(({lineNumber, count}) => { Nitpick: call this variable "line" for consistency with the loop above @@ +397,1 @@ * line: The line at which execution first started on this function. Now some parameters are documented as @param, and some without the @. Would be nice to be consistent. @@ +405,3 @@ + let hitCount = 0; + // The number of times the first opcode was executed is the number of times + // the function was executed? Stray question mark?
Review of attachment 360644 [details] [review]: ::: modules/_bootstrap/coverage.js @@ +395,3 @@ * } * @key: a unique key for the function whose coverage is being determined + * @line: The line at which this function is defined. Oh, this should probably go in the previous commit
Review of attachment 360689 [details] [review]: Nice
Review of attachment 360692 [details] [review]: OK
Wellll... I think these patches are actually obsolete as well now. I discovered that SpiderMonkey has also gained API for simply outputting an LCOV file (http://searchfox.org/mozilla-central/source/js/src/jsfriendapi.h#1163) and it seems to do exactly what we need, including branch coverage, with no custom code at all except for switching on Debugger.collectCoverageInfo. So I will simply remove all of this code and use that instead :-P
(In reply to Philip Chimento from comment #47) > Wellll... I think these patches are actually obsolete as well now. I > discovered that SpiderMonkey has also gained API for simply outputting an > LCOV file > (http://searchfox.org/mozilla-central/source/js/src/jsfriendapi.h#1163) and > it seems to do exactly what we need, including branch coverage, with no > custom code at all except for switching on Debugger.collectCoverageInfo. So > I will simply remove all of this code and use that instead :-P Oh nice. It'd be good to see what delta there is between this and what our tests expect.
Review of attachment 360692 [details] [review]: r+ for me too
Attachment 360644 [details] pushed as 0e1d86c - coverage: Misc refactors Attachment 360692 [details] pushed as 6ce9e42 - coverage: Fix coverage prefixes
I committed the above two patches to gnome-3-26 for today's 1.50.1, but I'm not going to commit them to master - instead I'll keep working on using js::GetCodeCoverageSummary(). It's working fairly well, though I still have to fix up some of the tests because the expected output is not exactly the same (branch numbers are sometimes reversed.)
This is ready for review again, but I am excited to invite you to review the GitLab merge request instead of Bugzilla patches! https://gitlab.gnome.org/GNOME/gjs/merge_requests/1
(In reply to Philip Chimento from comment #52) > This is ready for review again, but I am excited to invite you to review the > GitLab merge request instead of Bugzilla patches! > > https://gitlab.gnome.org/GNOME/gjs/merge_requests/1 \o/ I started reviewing it there. Thanks for working on this!
This was reviewed and merged over at GitLab.