Re: [PATCH 0/1] contrib: Add script to show uncovered "new" lines
"Derrick Stolee via GitGitGadget" writes: > There have been a few bugs in recent patches what would have been caught if > the test suite covered those blocks (including a few of mine). I want to > work towards a "sensible" amount of coverage on new topics. In my opinion, > this means that any logic should be covered, but the 'die()' blocks in error > cases do not need to be covered. Nice. > It is important to not measure the coverage of the codebase by what old code > is not covered. To help, I created the 'contrib/coverage-diff.sh' script. > After creating the coverage statistics at a version (say, 'topic') you can > then run > > contrib/coverage-diff.sh base topic > ... > Using this 'git blame' output, we can quickly inspect whether the uncovered > lines are appropriate. For instance: > ... > I used this approach for 'next' over 'master' and got a larger list, some of > which I have already submitted tests to increase coverage [2] or will be > covered by topics not in 'next' [3]. > > Thanks, -Stolee Thanks for working on this.
Re: [PATCH 0/1] contrib: Add script to show uncovered "new" lines
On 9/12/2018 12:45 PM, Derrick Stolee via GitGitGadget wrote: We have coverage targets in our Makefile for using gcov to display line coverage based on our test suite. The way I like to do it is to run: make coverage-test make coverage-report Very nice, I was unaware of the coverage test make targets. I like the new report; it makes it easier to verify any new changes are actually tested. 4. The lines in read-cache.c are part of a new block for the condition "if (expand_name_field)" as part of an optimization. These lines should probably be covered before that series is merged to 'next'. I understand that Ben and Duy are continuing work in this direction [1]. This code is only exercised when the index format is V4 but the default is version 2/3 [1]. To enable the test suite to use version 4 and test those code paths will require the addition of a new GIT_TEST_INDEX_VERSION environment variable. I'll add that to my TODO list. [1] https://git-scm.com/docs/git/2.1.0
Re: [PATCH 0/1] contrib: Add script to show uncovered "new" lines
On 9/12/2018 12:45 PM, Derrick Stolee via GitGitGadget wrote:
For example, I ran this against the 'jch' branch (d3c0046)
versus 'next' (dd90340)
As another example, I ran this against the 'pu' branch (4c416a53) versus
'jch' (d3c0046) and got the following output, submitted here without
commentary:
builtin/bisect--helper.c
0b1f0fd910c (Pranit Bauva 2017-10-27 15:06:37 +
43) free((void *) terms->term_good);
3d3237b0e6b (Pranit Bauva 2017-10-27 15:06:37 +
162) if (get_oid_commit(commit, ))
3d3237b0e6b (Pranit Bauva 2017-10-27 15:06:37 +
163) return error(_("'%s' is not a valid commit"),
commit);
3d3237b0e6b (Pranit Bauva 2017-10-27 15:06:37 +
164) strbuf_addstr(, commit);
3d3237b0e6b (Pranit Bauva 2017-10-27 15:06:37 +
172) error(_("Could not check out original HEAD '%s'.
Try "
3d3237b0e6b (Pranit Bauva 2017-10-27 15:06:37 +
174) strbuf_release();
3d3237b0e6b (Pranit Bauva 2017-10-27 15:06:37 +
175) argv_array_clear();
3d3237b0e6b (Pranit Bauva 2017-10-27 15:06:37 +
176) return -1;
0b1f0fd910c (Pranit Bauva 2017-10-27 15:06:37 +
215) error(_("Bad bisect_write argument: %s"), state);
0b1f0fd910c (Pranit Bauva 2017-10-27 15:06:37 +
216) goto fail;
0b1f0fd910c (Pranit Bauva 2017-10-27 15:06:37 +
220) error(_("couldn't get the oid of the rev '%s'"), rev);
0b1f0fd910c (Pranit Bauva 2017-10-27 15:06:37 +
221) goto fail;
0b1f0fd910c (Pranit Bauva 2017-10-27 15:06:37 +
226) goto fail;
0b1f0fd910c (Pranit Bauva 2017-10-27 15:06:37 +
230) error_errno(_("couldn't open the file '%s'"),
git_path_bisect_log());
0b1f0fd910c (Pranit Bauva 2017-10-27 15:06:37 +
231) goto fail;
0b1f0fd910c (Pranit Bauva 2017-10-27 15:06:37 + 242)fail:
0b1f0fd910c (Pranit Bauva 2017-10-27 15:06:37 + 243) retval
= -1;
a919f328ba3 (Pranit Bauva 2017-10-27 15:06:37 +
323) yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO);
a919f328ba3 (Pranit Bauva 2017-10-27 15:06:37 +
324) if (starts_with(yesno, "N") || starts_with(yesno, "n"))
a919f328ba3 (Pranit Bauva 2017-10-27 15:06:37 +
327) goto finish;
a919f328ba3 (Pranit Bauva 2017-10-27 15:06:37 +
336) error(_("You need to start by \"git bisect start\". You "
a919f328ba3 (Pranit Bauva 2017-10-27 15:06:37 +
341) goto fail;
a919f328ba3 (Pranit Bauva 2017-10-27 15:06:37 + 345)fail:
35f7ca528ae (Pranit Bauva 2017-10-27 15:06:37 +
387) return error(_("--bisect-term requires exactly one
argument"));
35f7ca528ae (Pranit Bauva 2017-10-27 15:06:37 +
400) error(_("BUG: invalid argument %s for 'git
bisect terms'.\n"
5dfeec316b8 (Pranit Bauva 2017-10-27 15:06:37 +
416) return -1;
5dfeec316b8 (Pranit Bauva 2017-10-27 15:06:37 +
419) goto fail;
5dfeec316b8 (Pranit Bauva 2017-10-27 15:06:37 +
423) goto fail;
5dfeec316b8 (Pranit Bauva 2017-10-27 15:06:37 + 427)fail:
5dfeec316b8 (Pranit Bauva 2017-10-27 15:06:37 + 428) retval
= -1;
5dfeec316b8 (Pranit Bauva 2017-10-27 15:06:37 +
466) no_checkout = 1;
5dfeec316b8 (Pranit Bauva 2017-10-27 15:06:37 +
488) !one_of(arg, "--term-good", "--term-bad", NULL)) {
5dfeec316b8 (Pranit Bauva 2017-10-27 15:06:37 +
489) return error(_("unrecognised option: '%s'"), arg);
5dfeec316b8 (Pranit Bauva 2017-10-27 15:06:37 +
523) if (get_oid("HEAD", _oid))
5dfeec316b8 (Pranit Bauva 2017-10-27 15:06:37 +
524) return error(_("Bad HEAD - I need a HEAD"));
5dfeec316b8 (Pranit Bauva 2017-10-27 15:06:37 +
539) error(_("checking out '%s' failed. Try
'git "
5dfeec316b8 (Pranit Bauva 2017-10-27 15:06:37 +
559) return error(_("won't bisect on
cg-seek'ed tree"));
5dfeec316b8 (Pranit Bauva 2017-10-27 15:06:37 +
562) return error(_("Bad HEAD - strange symbolic ref"));
5dfeec316b8 (Pranit Bauva 2017-10-27 15:06:37 +
570) return -1;
5dfeec316b8 (Pranit Bauva 2017-10-27 15:06:37 +
588) goto fail;
5dfeec316b8 (Pranit Bauva 2017-10-27 15:06:37 +
598) goto fail;
5dfeec316b8 (Pranit Bauva 2017-10-27 15:06:37 +
606) goto fail;
3d3237b0e6b (Pranit Bauva 2017-10-27 15:06:37 +
686) return error(_("--bisect-reset requires either
no argument or a commit"));
0b1f0fd910c
[PATCH 0/1] contrib: Add script to show uncovered "new" lines
We have coverage targets in our Makefile for using gcov to display line
coverage based on our test suite. The way I like to do it is to run:
make coverage-test
make coverage-report
This leaves the repo in a state where every X.c file that was covered has an
X.c.gcov file containing the coverage counts for every line, and "#" at
every uncovered line.
There have been a few bugs in recent patches what would have been caught if
the test suite covered those blocks (including a few of mine). I want to
work towards a "sensible" amount of coverage on new topics. In my opinion,
this means that any logic should be covered, but the 'die()' blocks in error
cases do not need to be covered.
It is important to not measure the coverage of the codebase by what old code
is not covered. To help, I created the 'contrib/coverage-diff.sh' script.
After creating the coverage statistics at a version (say, 'topic') you can
then run
contrib/coverage-diff.sh base topic
to see the lines added between 'base' and 'topic' that are not covered by
the test suite. For example, I ran this against the 'jch' branch (d3c0046)
versus 'next' (dd90340) and got the following output:
builtin/commit.c
859fdc0c3cf (Derrick Stolee 2018-08-29 05:49:04 -0700 1657)
write_commit_graph_reachable(get_object_directory(), 0);
builtin/rev-list.c
250edfa8c87 (Harald Nordgren2018-04-18 23:05:35 +0200 431)
bisect_flags |= BISECT_FIND_ALL;
builtin/worktree.c
e5353bef550 (Eric Sunshine 2018-08-28 17:20:19 -0400 60)
error_errno(_("failed to delete '%s'"), sb.buf);
e19831c94f9 (Eric Sunshine 2018-08-28 17:20:23 -0400 251)
die(_("unable to re-add worktree '%s'"), path);
68a6b3a1bd4 (Eric Sunshine 2018-08-28 17:20:24 -0400 793)
die(_("cannot move a locked working tree, lock reason: %s\nuse 'move -f
-f' to override or unlock first"),
f4143101cbb (Eric Sunshine 2018-08-28 17:20:25 -0400 906)
die(_("cannot remove a locked working tree, lock reason: %s\nuse
'remove -f -f' to override or unlock first"),
read-cache.c
67922a3 (Nguyễn Thái Ngọc Duy 2018-09-02 15:19:33 +0200 1754)
const unsigned char *cp = (const unsigned char *)name;
67922a3 (Nguyễn Thái Ngọc Duy 2018-09-02 15:19:33 +0200 1757)
previous_len = previous_ce ? previous_ce->ce_namelen : 0;
67922a3 (Nguyễn Thái Ngọc Duy 2018-09-02 15:19:33 +0200 1758)
strip_len = decode_varint();
67922a3 (Nguyễn Thái Ngọc Duy 2018-09-02 15:19:33 +0200 1759)
if (previous_len < strip_len) {
67922a3 (Nguyễn Thái Ngọc Duy 2018-09-02 15:19:33 +0200 1760)
if (previous_ce)
67922a3 (Nguyễn Thái Ngọc Duy 2018-09-02 15:19:33 +0200 1761)
die(_("malformed name field in the index, near path
'%s'"),
67922a3 (Nguyễn Thái Ngọc Duy 2018-09-02 15:19:33 +0200 1762)
previous_ce->name);
67922a3 (Nguyễn Thái Ngọc Duy 2018-09-02 15:19:33 +0200 1764)
die(_("malformed name field in the index in the first
path"));
67922a3 (Nguyễn Thái Ngọc Duy 2018-09-02 15:19:33 +0200 1766)
copy_len = previous_len - strip_len;
67922a3 (Nguyễn Thái Ngọc Duy 2018-09-02 15:19:33 +0200 1767)
name = (const char *)cp;
67922a3 (Nguyễn Thái Ngọc Duy 2018-09-02 15:19:33 +0200 1773)
len += copy_len;
67922a3 (Nguyễn Thái Ngọc Duy 2018-09-02 15:19:33 +0200 1794)
if (copy_len)
67922a3 (Nguyễn Thái Ngọc Duy 2018-09-02 15:19:33 +0200 1795)
memcpy(ce->name, previous_ce->name, copy_len);
67922a3 (Nguyễn Thái Ngọc Duy 2018-09-02 15:19:33 +0200 1796)
memcpy(ce->name + copy_len, name, len + 1 - copy_len);
67922a3 (Nguyễn Thái Ngọc Duy 2018-09-02 15:19:33 +0200 1797)
*ent_size = (name - ((char *)ondisk)) + len + 1 - copy_len;
remote-curl.c
c3b9bc94b9b (Elijah Newren 2018-09-05 10:03:07 -0700 181)
options.filter = xstrdup(value);
Using this 'git blame' output, we can quickly inspect whether the uncovered
lines are appropriate. For instance:
1. The line in builtin/commit.c is due to writing the commit-graph file
when GIT_TEST_COMMIT_GRAPH is enabled, which is not on by default in the
test suite. Being uncovered is expected here.
2. The lines in builtin/worktree.c are all related to error conditions.
This is acceptable.
3. The line in builtin/rev-list.c is a flag replacement in a block that is
otherwise unchanged. It must not be covered by the test suite normally.
This could be worth adding a test to ensure the new logic maintains old
behavior.
4. The lines in
