Implement branch coverage - #9751
Conversation
|
Thanks! Will run this through the review mill. |
|
@headius Thanks! Do you think this will also make it into the JRuby 10.1.3.0 release? |
|
@sferik Sure! JRuby 10.1.3.0 isn't scheduled right now. Perhaps a little motivation to release soon given some CVEs in BouncyCastle (mitigated by jruby-openssl 0.16.3) but we've got some momentum here to keep merging stuff. |
4d11dbb to
82332cc
Compare
headius
left a comment
There was a problem hiding this comment.
Looks like a good chunk. I've added some specific comments and change requests.
The complexity of the PR comes primarily from two places:
- Adding parser and AST support for tracking more source information about branch arms.
- Propagating branch information through all of the IR builder methods and adding compilation for branch coverage instructions when appropriate.
General idea is that AST builds up the data on the source ranges for branches and their arms, IRBuilder sets up the global state for tracking coverage against those branches while compile, and the branch coverage instructions (and jitted code) update that state.
BIG open question here for me is what this looks like with Prism. Eventually, we hope to adopt it as the default. Presumably what we get out of Prism already has this info, or it can be inferred.
There's room for refinement here but if we can address my comments this will be good to merge.
Coverage's branches mode now works and produces the same results as CRuby
(its default, prism-based compiler):
{ file => { branches: {
[:if, 0, 2, 2, 6, 5] => { [:then, 1, 3, 4, 3, 5] => 2, [:else, 2, 5, 4, 5, 5] => 1 },
... } } }
Every branching construct (if, unless, elsif chains, ternaries, modifiers,
case/when, case/in, while, until, their modifier forms including
begin/end while, and safe navigation calls and assignments) is reported with
the same keys, locations, numbering and counts as CRuby, including its
corner cases: empty arms and missing else clauses, a clause's own span when
its body is empty, elsif clauses reaching the shared end, loop bodies that
never run, nil paths of every call in a &. chain, parenthesized arms, and
conditionals on literal predicates, which CRuby folds away together with
everything inside the arm that cannot run.
Design, following the same shape as method coverage:
- BranchCoverage / BranchTarget hold one branching construct and the places
it branches to, each target owning a lock-free atomic counter; FileCoverage
keeps them in declaration order and identifies them by type and span, so
building the same source again (a reloaded file, a block turned into a
method) finds the existing counters rather than declaring new ones. A
result is built from a copy taken under the CoverageData lock, which
reads and clears each count in one atomic step, so a branch reached
while Coverage.result(clear: true) runs is reported once, not dropped.
- The IR builder declares constructs while it builds their IR, in the order
CRuby's compiler meets them (an if after its predicate, a loop or case
before, a safe navigation after its receiver), and emits a
CoverBranchInstr at the start of each arm. The instruction holds the
counter directly; JIT-compiled code binds to it through an invokedynamic
site keyed by file and target index. Methods are built eagerly while
branches are measured so a file's result lists every branch, and lazy
&. chains no longer share a nil exit so each call's nil path counts.
- The parser records a source span on every AST node: the generated parser
now gives each node the location of the production that produced it
(widened by enclosing productions that hand the node along, until an
explicit span locks it; calls keep the span they were created with so an
attached block is not part of them), and empty productions get bison's
zero-width location instead of a stale one. The grammar records the extra
positions branch keys need (where a predicate ends, where an else keyword
starts, a when/in clause through its then, a modifier's statement as
written, the parentheses around an arm) and whether a conditional is one
CRuby folds.
Coverage.supported?(:branches) is now true and the "branch coverage is not
supported" warning is gone. Un-excludes the MRI branch coverage tests
(including test_coverage_supported, test_coverage_ensure_if_return and
test_tag_break_with_branch_coverage) and adds JRuby tests for keys and
counts, result shape, suspend/resume/clear, exact counts under parallel
calls, reloaded files and define_method blocks, and every execution mode.
See jruby#5147
SimpleCov checks the branches it synthesizes for files that were never
loaded against what Coverage reports, construct by construct, and on
JRuby 18 of its 73 constructs disagreed with CRuby:
- A literal predicate folds only where MRI's compiler folds it. Inside
parentheses MRI keeps a node of its own, which folds only when it
compiles to a single constant: (1; 2) and (a; [1]; 2) fold by their
last statement when nothing before it compiles to anything, while
(nil), ("x") and (-> {}) keep their branch. __ENCODING__ and a bare
lambda fold. && and || follow which of the two labels can be jumped
to, so a || 1 folds but nil && 1 does not.
- Nothing in an arm that can never run is measured, which now includes
the blocks, lambdas, classes and methods written there.
- expr => pattern and expr in pattern report no branch, as from Ruby
3.4 on.
A construct's type and a target's label were strings, and both were looked up by a string built from them and their span. The type is now BranchCoverage.Type and the label BranchTarget.Label, each carrying the name Coverage reports it as, and declared constructs and targets are looked up by a record of the type or label and the span.
buildConditional took the arm that can never run as 0, 1 or -1. It is now IRBuilder.DeadArm: STATEMENTS, CONSEQUENT or NONE.
82332cc to
c8a43c3
Compare
|
Thanks for the review! I've addressed the inline comments in 16a2e9c and c8a43c3. On Prism. most of this PR doesn't depend on which parser is used. The counters ( That part should be simpler with Prism. The keys follow CRuby’s Prism compiler, so the positions come straight from Prism’s node locations ( |
SimpleCov warned on every JRuby run without --debug that coverage may be inaccurate, citing jruby/jruby#1196 from the JRuby 1.7 days. JRuby's coverage no longer depends on full-trace mode. The whole suite on JRuby 10.0.7, run with and without --debug, reports every line of SimpleCov's own code identically: the same lines relevant, the same lines hit. With the branch and line coverage work in jruby/jruby#9751 and jruby/jruby#9682 the same holds for every line, branch, and method. --debug also slows JRuby down, so the warning only cost the people who followed it. Drop the warning, the advice in the troubleshooting guide, and JRUBY_OPTS=--debug from CI.
define_method(:new_name, some_method) adds a copy of some_method that keeps its name. MRI keys the coverage entry by the method's original_id, so for class A; def foo; end; end class B < A; define_method(:bar, A.instance_method(:foo)); end it reports [B, :foo, ...], not [B, :bar, ...], on every version from 3.3 to master (rb_coverage_method_data_of in thread.c). This reverts the keying by the added name from caf18c6, which assumed the opposite.
MRI gives a file new line and branch counters each time it is loaded (iseq_setup_coverage replaces the path's entry), while an eval that names an already covered file counts into that file's entry. JRuby merged every parse of a file into the same entry, so loading a file twice added the counts of both loads together. Only an eval now merges into an existing entry. A load clears the file's lines and branches; the branch targets stay registered, since code from the earlier load finds them by index. Method entries are kept, as MRI sums the calls of the methods defined at the same place.
headius
left a comment
There was a problem hiding this comment.
Not sure when we're going to get reviews from @enebo so I'll make the executive decision and go forward with this for now. We can do some testing to see how memory size is impacted. We do compile to IR but we do so lazily, leaving most AST nodes in memory until they will actually be executed.
This adds the
branchesmode of theCoveragelibrary. Results have the same shape, keys, numbering and counts as CRuby (its default, prism-based compiler):Keys are
[type, id, start_line, start_column, end_line, end_column]for each construct and[label, id, start_line, start_column, end_line, end_column]for each of its targets, with ids numbered consecutively across the file in the order CRuby's compiler meets them.Addresses the branch half of #5147, following up on #9676, which added method coverage.
Every branching construct is covered:
if,unless,elsifchains, ternaries, modifier forms,case/when,case/in,while,until(includingbegin/end while), and safe navigation calls and assignments. CRuby's corner cases are matched too:elseclauses, and a clause's own span when its body is emptyelsifclauses reaching the sharedend&.chain (lazy&.chains no longer share a single nil exit)BranchCoverageandBranchTargethold one branching construct and the places it branches to.FileCoveragekeeps them in declaration order and identifies them by type and span, so building the same source again (a reloaded file, a block turned into a method bydefine_method) finds the existing counters rather than declaring new ones.The IR builder is where branches are marked, as suggested by @enebo in #5147 (comment), rather than recovered from the IR's own jumps:
ifafter its predicate, a loop orcasebefore, a safe navigation after its receiver), so nested constructs are numbered the same wayCoverBranchInstris emitted at the start of each arm and holds the counter directly. JIT-compiled code binds to it through an invokedynamic site (CoverageSite) keyed by file and target index, which becomes a no-op if the file is no longer being measured.Counting is a single lock-free atomic add (
VarHandle#getAndAdd), as with method coverage. A result is built from a snapshot taken under theCoverageDatalock, which reads and resets each count in one atomic step, so a branch reached whileCoverage.result(clear: true)runs is reported once rather than dropped.The parser now records a source span on every AST node. The generated parser gives each node the location of the production that produced it, widened by enclosing productions that hand the node along until an explicit span locks it (calls keep the span they were created with, so an attached block is not part of them), and empty productions get bison's zero-width location instead of a stale one. The grammar also records the extra positions branch keys need: where a predicate ends, where an
elsekeyword starts, awhen/inclause through itsthen, a modifier's statement as written, the parentheses around an arm, and whether a conditional is one CRuby folds. Both generated parsers were regenerated fromRubyParser.yandskeleton.parserwith jay.Coverage.supported?(:branches)is now true and the "branch coverage is not supported" warning is gone.Cc: @kares @headius