Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes to gcovr have broken node-core-coverage #19

Closed
CurryKitten opened this issue Aug 9, 2016 · 1 comment
Closed

Changes to gcovr have broken node-core-coverage #19

CurryKitten opened this issue Aug 9, 2016 · 1 comment

Comments

@CurryKitten
Copy link
Contributor

CurryKitten commented Aug 9, 2016

We've been using this to test code coverage over the last week, and I noticed this morning that some updates to gcovr have broken node-core-coverage in two ways.

Firstly the changes in gcovr's script/gcovr mean that the last part of the gcovr-patches.diff file, i.e

@@ -2155,6 +2158,7 @@ if options.root is not None:
 else:
     root_dir = starting_dir
     options.root_filter = re.compile(re.escape(root_dir + os.sep))
+    starting_dir = root_dir

 for i in range(0, len(options.filter)):
     options.filter[i] = re.compile(options.filter[i])

Fail to apply, although I've run coverage.sh without this part of the patch file, and it appears to run through without a problem.

Secondly, the grep line for gathering c++ coverage -

CXXCOVERAGE=$(grep -A3 Lines coverage/cxxcoverage.html | \
  grep style|grep -o '[0-9\.]*')

Runs into a problem as it now picks up part of the background colour from the CSS. i.e from the line -

<td class="headerTableEntry" style="background-color:#FFFF55">84.2 %</td>

we get (back from the grep) -

55
84.2

Which in turn writes the wrong format out to out/index.csv and causes generate-index-html.py to fall over. I fixed this locally by replacing the code with -

CXXCOVERAGE=$(grep -A3 Lines coverage/cxxcoverage.html | \
  grep style|grep -o '[0-9]\{1,3\}\.[0-9]\{1,2\}')

I've also been testing by passing in the branch I wanted to run the coverage tests on, rather than just cloning the latest master. I wondered if you were open to having this sort of option in the code ?

@addaleax
Copy link
Owner

addaleax commented Aug 9, 2016

Hi! Yeah, I’ve been noticing. This is broken with Node’s master right now anyway because there’s ES6 syntax in it that istanbul doesn’t quite understand, so I’m looking into upgrading everything today.

The gcovr-patches.diff bit that doesn’t apply… yeah, it’s weird. I have no idea why I did that in the first place, it doesn’t seem to make sense at all?

I’ll try to get everything fixed again today, but feel completely free to PR against this repo!

I wondered if you were open to having this sort of option in the code ?

Sure! Again, PRs are always welcome.

addaleax pushed a commit that referenced this issue Aug 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants