Fix always FULLY COVERED

[benjaminfuchs]

First of all, very cool tool! 👍
I just tried it with a little gcov example today and always got the message FULLY COVERED. I tried my best and guess I found the bug:

BEFORE CHANGE

gcov_example $ git-coverage  
FULLY COVERED FILE: main.c

AFTER CHANGE

gcov_example $ git-coverage  
--- a/main.c
+++ b/main.c
@@ -32,6 +32,7 @@ int main () {
      }
      else {
!+        cout << "NEW DEAD CODE";
      }
  
      return 0;

GCOV VERSION

gcov_example $ gcov -v  
gcov (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.
There is NO warranty; not even for MERCHANTABILITY or 
FITNESS FOR A PARTICULAR PURPOSE.

[BobBall]

Thanks for the pull request. I apologies that we weren't clear with the repository in the first place, but this is actually a clone from http://stef.thewalter.net/git-coverage-useful-code-coverage.html

It's not clear to me how to make a pull request to the repository listed above, so I'll accept the pull request here for completeness.

Thanks for fixing the C paths - we're using only the python path for the coverage, which is why we didn't spot it :)

[BobBall]

My reading is that gcov output should give lines like:
1: 12: if (total != 45)
#####: 13: printf ("Failure\n");
-: 14: else
1: 15: printf ("Success\n");

In this case, "covered" will fail to convert to an integer where ##### is present. As such, it will skip to the ValueError clause, and only add the line to 'coverage' if
I think we want coverage[] to be set as covered if either a number or the '-' is present, so we don't report the branch statement as being not covered.
As such, it seems to me that the logic is to "mark as covered" if: We have a count or a '-' output from gcov, or the source line matches one of the skips.

I think the code follows that logic currently, so I'm not convinced by this change - can you explain why it works?

[benjaminfuchs]

Okay it seems to be a bit more tricky....
Here is my file:

/*
 * Example for use of gcov
 * 
 */

#include <iostream>

using namespace std;

class Dummy {
public:
    void func_a() { 
        cout << "Hello \n";
    }
    void func_b() {
        cout << "World!\n";
    }
    int func_c() { return 1; }

};

int main () {
    Dummy oDummy;

    int c = oDummy.func_c();

    if(c)
    {
        oDummy.func_a();
    } else if (c > 2) {
        oDummy.func_b();
    }
    else {
        cout << "DEAD CODE";
        cout << "DEAD CODE";
    }

    return 0;
}

If I add a print(line) to the coverage function I get following output:

/home/fuchs/github/gcov_example/main.c.gcov
        -:    0:Source:main.c
        -:    0:Graph:/home/fuchs/github/gcov_example/main.gcno
        -:    0:Data:/home/fuchs/github/gcov_example/main.gcda
        -:    0:Runs:1
        -:    0:Programs:1
        -:    1:/*
        -:    2: * Example for use of gcov
        -:    3: *
        -:    4: */
        -:    5:
        -:    6:#include <iostream>
        -:    7:
        -:    8:using namespace std;
        -:    9:
        -:   10:class Dummy {
        -:   11:public:
        1:   12:    void func_a() {
        1:   13:        cout << "Hello \n";
        1:   14:    }
    #####:   15:    void func_b() {
    #####:   16:        cout << "World!\n";
    #####:   17:    }
        1:   18:    int func_c() { return 1; }
        -:   19:
        -:   20:};
        -:   21:
        1:   22:int main () {
        -:   23:    Dummy oDummy;
        -:   24:
        1:   25:    int c = oDummy.func_c();
        -:   26:
        1:   27:    if(c)
        -:   28:    {
        1:   29:        oDummy.func_a();
    #####:   30:    } else if (c > 2) {
    #####:   31:        oDummy.func_b();
        -:   32:    }
        -:   33:    else {
    #####:   34:        cout << "DEAD CODE";
    #####:   35:        cout << "DEAD CODE";
        -:   36:    }
        -:   37:
        1:   38:    return 0;
        3:   39:}
/home/fuchs/github/gcov_example/#usr#include#c++#5#iostream.gcov
        -:    0:Source:/usr/include/c++/5/iostream
        -:    0:Graph:/home/fuchs/github/gcov_example/main.gcno
        -:    0:Data:/home/fuchs/github/gcov_example/main.gcda
        -:    0:Runs:1
        -:    0:Programs:1
        -:    1:// Standard iostream objects -*- C++ -*-
        -:    2:
        -:    3:// Copyright (C) 1997-2015 Free Software Foundation, Inc.
        -:    4://
        -:    5:// This file is part of the GNU ISO C++ Library.  This library is free
        -:    6:// software; you can redistribute it and/or modify it under the
        -:    7:// terms of the GNU General Public License as published by the
        -:    8:// Free Software Foundation; either version 3, or (at your option)
        -:    9:// any later version.
        -:   10:
        -:   11:// This library is distributed in the hope that it will be useful,
        -:   12:// but WITHOUT ANY WARRANTY; without even the implied warranty of
        -:   13:// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
        -:   14:// GNU General Public License for more details.
        -:   15:
        -:   16:// Under Section 7 of GPL version 3, you are granted additional
        -:   17:// permissions described in the GCC Runtime Library Exception, version
        -:   18:// 3.1, as published by the Free Software Foundation.
        -:   19:
        -:   20:// You should have received a copy of the GNU General Public License and
        -:   21:// a copy of the GCC Runtime Library Exception along with this program;
        -:   22:// see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
        -:   23:// <http://www.gnu.org/licenses/>.
        -:   24:
        -:   25:/** @file include/iostream
        -:   26: *  This is a Standard C++ Library header.
        -:   27: */
        -:   28:
        -:   29://
        -:   30:// ISO C++ 14882: 27.3  Standard iostream objects
        -:   31://
        -:   32:
        -:   33:#ifndef _GLIBCXX_IOSTREAM
        -:   34:#define _GLIBCXX_IOSTREAM 1
        -:   35:
        -:   36:#pragma GCC system_header
        -:   37:
        -:   38:#include <bits/c++config.h>
        -:   39:#include <ostream>
        -:   40:#include <istream>
        -:   41:
        -:   42:namespace std _GLIBCXX_VISIBILITY(default)
        -:   43:{
        -:   44:_GLIBCXX_BEGIN_NAMESPACE_VERSION
        -:   45:
        -:   46:  /**
        -:   47:   *  @name Standard Stream Objects
        -:   48:   *
        -:   49:   *  The &lt;iostream&gt; header declares the eight <em>standard stream
        -:   50:   *  objects</em>.  For other declarations, see
        -:   51:   *  http://gcc.gnu.org/onlinedocs/libstdc++/manual/io.html
        -:   52:   *  and the @link iosfwd I/O forward declarations @endlink
        -:   53:   *
        -:   54:   *  They are required by default to cooperate with the global C
        -:   55:   *  library's @c FILE streams, and to be available during program
        -:   56:   *  startup and termination. For more information, see the section of the
        -:   57:   *  manual linked to above.
        -:   58:  */
        -:   59:  //@{
        -:   60:  extern istream cin;       /// Linked to standard input
        -:   61:  extern ostream cout;      /// Linked to standard output
        -:   62:  extern ostream cerr;      /// Linked to standard error (unbuffered)
        -:   63:  extern ostream clog;      /// Linked to standard error (buffered)
        -:   64:
        -:   65:#ifdef _GLIBCXX_USE_WCHAR_T
        -:   66:  extern wistream wcin;     /// Linked to standard input
        -:   67:  extern wostream wcout;    /// Linked to standard output
        -:   68:  extern wostream wcerr;    /// Linked to standard error (unbuffered)
        -:   69:  extern wostream wclog;    /// Linked to standard error (buffered)
        -:   70:#endif
        -:   71:  //@}
        -:   72:
        -:   73:  // For construction of filebuffers for cout, cin, cerr, clog et. al.
        1:   74:  static ios_base::Init __ioinit;
        -:   75:
        -:   76:_GLIBCXX_END_NAMESPACE_VERSION
        -:   77:} // namespace
        -:   78:
        -:   79:#endif /* _GLIBCXX_IOSTREAM */
FULLY COVERED FILE: main.c

So seems my fix was fixing the wrong place.
What confuese me is the #usr#include#c++#5#iostream.gcov
Maybe there is a wrong coding or something like that.

gcovs prints (and thats what the script parses wrong):

File 'main.c'
Lines executed:60.00% of 10
Creating 'main.c.gcov'

File '/usr/include/c++/5/iostream'
Lines executed:100.00% of 1
Creating 'iostream.gcov'

File 'dummy.c'
Lines executed:57.14% of 7
Creating 'dummy.c.gcov'

[benjaminfuchs]

The latest fix seems to be at the right place, but still maybe not the final one. If I have some more time I will try to look for a real fix. Or maybe you have an idea?
Greating

[BobBall]

OK, so I don't know what the right fix is ATM but it seems that the gcov output is covering multiple files.
In my test run, line 39 was covered by:
-: 39:#include <bits/stl_algobase.h> // std::copy, std::fill_n
which seems to come from:
-: 0:Source:/usr/include/c++/4.9/bits/char_traits.h
-: 0:Graph:/local/scratch/bobba/tmp/gcov.gcno
-: 0:Data:/local/scratch/bobba/tmp/gcov.gcda
-: 0:Runs:1
-: 0:Programs:1

I therefore assume that the fix that is needed is to make _gcov_lines_for_files to actually only return the gcov lines for the given file.

[BobBall]

I don't really understand how to use gcov, but I think from my playing around that the following change might fix it: I think that the _gcov_lines_for_files is listing all files from the single gcno file, and it's not actually filtering a single gcno file based on the input filename, despite what the function claims to do.

@@ -398,6 +401,8 @@
                 if not match:
                     continue
                 gcov = match.group(1)
+                if gcov != "%s.gcov"%filename:
+                    continue
                 if os.path.exists(gcov):
                     gcovs.append(os.path.abspath(gcov))

[benjaminfuchs]

Just what I am looking at, I added just two print statements:

390         for gcno in self._lookup_gcno_files(filename):
391             # Need to run gcov in the same directory compiled in
392             directory = self._directory_gcno_compiled_in(gcno, filename)
393             oldcwd = os.getcwd()
394             os.chdir(directory)
395 
396             gcovs = []
397             cmd = ['gcov', '--preserve-paths']
398             for line in subprocess_lines(cmd + [gcno]):
399                 match = self._creating_re.match(line.strip())
400                 if not match:
401                     continue
402                 gcov = match.group(1)
403                 print(gcov)
404                 if os.path.exists(gcov):
405                     print(gcov)
406                     gcovs.append(os.path.abspath(gcov))

And toggeld this comment:

 19 def subprocess_lines(argv):
 20     proc = subprocess.Popen(argv, stdout=subprocess.PIPE)
 21     while True:
 22         line = proc.stdout.readline().replace("#","/")
 23         #line = proc.stdout.readline()
 24         if line != "":
 25             yield line
 26         else:
 27             return

And got this output:

gcov_example (master *) $ ../git-coverage/git-coverage 
main.c.gcov
main.c.gcov
#usr#include#c++#5#iostream.gcov
#usr#include#c++#5#iostream.gcov
dummy.c.gcov
dummy.c.gcov
FULLY COVERED FILE: main.c
gcov_example (master *) $ ../git-coverage/git-coverage 
main.c.gcov
main.c.gcov
/usr/include/c++/5/iostream.gcov
dummy.c.gcov
dummy.c.gcov
--- a/main.c
+++ b/main.c
@@ -22,6 +22,7 @@ int main () {
      }
      else {
          cout << "DEAD CODE";
!+        cout << "DEAD CODE";
      }

      return 0;

So it looks for me the os.path.exists(gcov) is giving a wrong true for the string with "#".
Seems the problem is caused maybe wrong parsing/coding of the Popen string in addition to a buggy path.exists?

[BobBall]

Hi, please see my suggested fix on #4 (comment)

os.path.exists works; the file does exist at the time the call is made. It is deleted at the end of _gcov_lines_for_files. Your comment makes me realise that my suggested patch is not complete because it does not delete files which are deemed irrelevant.

[benjaminfuchs]

Okay thanks! Shame on me to blame os.path.exists ;) Thanks for the fix.

[benjaminfuchs]

Okay, looked into it and it seems it still has a problem for includes in the main.c
Will look into it tomorrow.

[BobBall]

The latest fix looks good to me - what problems are showing up with includes from main.c?
Do they not get covered correctly?

[benjaminfuchs]

So I really had problem with coverage of includes.
I guess I fixed it, see below.
Repo to reproduce: https://github.com/benjaminfuchs/gcov-example.git

GIT DIFF

diff --git a/dummy.c b/dummy.c
index 30025e3..dd4f641 100644
--- a/dummy.c
+++ b/dummy.c
@@ -13,6 +13,7 @@ public:
         }
         else {
             cout << "DEAD CODE!\n";
+            cout << "DEAD BEEF!\n";
         }
     }
 };
diff --git a/main.c b/main.c
index c414003..d22237f 100644
--- a/main.c
+++ b/main.c
@@ -19,6 +19,7 @@ int main () {
     }
     else {
         cout << "DEAD CODE";
+        cout << "DEAD BEEF";
     }

     return 0;

BEFORE CHANGE

FULLY COVERED FILE: dummy.c
--- a/main.c
+++ b/main.c
@@ -19,6 +19,7 @@ int main () {
      }
      else {
          cout << "DEAD CODE";
!+        cout << "DEAD BEEF";
      }

      return 0;

AFTER CHANGE

--- a/dummy.c
+++ b/dummy.c
@@ -13,6 +13,7 @@ public:
          }
          else {
              cout << "DEAD CODE!\n";
!+            cout << "DEAD BEEF!\n";
          }
      }
  };
--- a/main.c
+++ b/main.c
@@ -19,6 +19,7 @@ int main () {
      }
      else {
          cout << "DEAD CODE";
!+        cout << "DEAD BEEF";
      }

      return 0;

[BobBall]

This is a string match against *.gcov; this shouldn't work.
It needs to be:
if gcov != "%s.gcov"%filename:
os.unlink(gcov)
continue