git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: peff@peff.net, Junio C Hamano <gitster@pobox.com>
Subject: [PATCH v2 0/1] contrib: Add script to show uncovered "new" lines
Date: Thu, 13 Sep 2018 07:56:42 -0700 (PDT)	[thread overview]
Message-ID: <pull.40.v2.git.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.40.git.gitgitgadget@gmail.com>

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 Nordgren        2018-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
67922abbbb3     (Nguyễn Thái Ngọc Duy   2018-09-02 15:19:33 +0200       1754)           const unsigned char *cp = (const unsigned char *)name;
67922abbbb3     (Nguyễn Thái Ngọc Duy   2018-09-02 15:19:33 +0200       1757)           previous_len = previous_ce ? previous_ce->ce_namelen : 0;
67922abbbb3     (Nguyễn Thái Ngọc Duy   2018-09-02 15:19:33 +0200       1758)           strip_len = decode_varint(&cp);
67922abbbb3     (Nguyễn Thái Ngọc Duy   2018-09-02 15:19:33 +0200       1759)           if (previous_len < strip_len) {
67922abbbb3     (Nguyễn Thái Ngọc Duy   2018-09-02 15:19:33 +0200       1760)                   if (previous_ce)
67922abbbb3     (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'"),
67922abbbb3     (Nguyễn Thái Ngọc Duy   2018-09-02 15:19:33 +0200       1762)                               previous_ce->name);
67922abbbb3     (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"));
67922abbbb3     (Nguyễn Thái Ngọc Duy   2018-09-02 15:19:33 +0200       1766)           copy_len = previous_len - strip_len;
67922abbbb3     (Nguyễn Thái Ngọc Duy   2018-09-02 15:19:33 +0200       1767)           name = (const char *)cp;
67922abbbb3     (Nguyễn Thái Ngọc Duy   2018-09-02 15:19:33 +0200       1773)                   len += copy_len;
67922abbbb3     (Nguyễn Thái Ngọc Duy   2018-09-02 15:19:33 +0200       1794)           if (copy_len)
67922abbbb3     (Nguyễn Thái Ngọc Duy   2018-09-02 15:19:33 +0200       1795)                   memcpy(ce->name, previous_ce->name, copy_len);
67922abbbb3     (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);
67922abbbb3     (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 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].
    
    

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

CHANGES IN V2: I converted the script from bash to sh (there may still be
POSIX-compliance issues with the new script, I don't know a lot when it
comes to that area). I also streamlined the machinery to add line numbers to
the diff. One downside is that the script runs a little slower with all of
the grep process invocations.

[1] 
https://public-inbox.org/git/20180912161832.55324-1-benpeart@microsoft.com/T/#u

[2] https://public-inbox.org/git/pull.37.git.gitgitgadget@gmail.com/

[3] https://public-inbox.org/git/pull.34.git.gitgitgadget@gmail.com/

Derrick Stolee (1):
  contrib: add coverage-diff script

 contrib/coverage-diff.sh | 63 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100755 contrib/coverage-diff.sh


base-commit: 1d4361b0f344188ab5eec6dcea01f61a3a3a1670
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-40%2Fderrickstolee%2Fcoverage-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-40/derrickstolee/coverage-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/40

Range-diff vs v1:

 1:  e4124471e5 < -:  ---------- contrib: add coverage-diff script
 -:  ---------- > 1:  7714b0659e contrib: add coverage-diff script

-- 
gitgitgadget

  parent reply	other threads:[~2018-09-13 14:56 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-12 16:45 [PATCH 0/1] contrib: Add script to show uncovered "new" lines Derrick Stolee via GitGitGadget
2018-09-12 16:45 ` [PATCH 1/1] contrib: add coverage-diff script Derrick Stolee via GitGitGadget
2018-09-12 22:13   ` Junio C Hamano
2018-09-12 22:54     ` Junio C Hamano
2018-09-13 12:21       ` Derrick Stolee
2018-09-13 14:59         ` Junio C Hamano
2018-09-12 19:14 ` [PATCH 0/1] contrib: Add script to show uncovered "new" lines Derrick Stolee
2018-09-12 19:33 ` Ben Peart
2018-09-12 19:53 ` Junio C Hamano
2018-09-13 14:56 ` Derrick Stolee via GitGitGadget [this message]
2018-09-13 14:56   ` [PATCH v2 1/1] contrib: add coverage-diff script Derrick Stolee via GitGitGadget
2018-09-13 17:40     ` Junio C Hamano
2018-09-13 18:15       ` Derrick Stolee
2018-09-13 17:54     ` Eric Sunshine
2018-09-21 15:15   ` [PATCH v3 0/1] contrib: Add script to show uncovered "new" lines Derrick Stolee via GitGitGadget
2018-09-21 15:15     ` [PATCH v3 1/1] contrib: add coverage-diff script Derrick Stolee via GitGitGadget
2018-09-25 18:36       ` Junio C Hamano
2018-09-21 15:20     ` [PATCH v3 0/1] contrib: Add script to show uncovered "new" lines Derrick Stolee
2018-10-08 14:52     ` [PATCH v4 " Derrick Stolee via GitGitGadget
2018-10-08 14:52       ` [PATCH v4 1/1] contrib: add coverage-diff script Derrick Stolee via GitGitGadget
2018-10-12  3:01       ` [PATCH v4 0/1] contrib: Add script to show uncovered "new" lines Junio C Hamano
2018-10-12 12:09         ` Derrick Stolee

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=pull.40.v2.git.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).