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

Build: Produce C/JS code coverage reports from make #10856

Closed
wants to merge 2 commits into from
Closed

Build: Produce C/JS code coverage reports from make #10856

wants to merge 2 commits into from

Conversation

CurryKitten
Copy link
Contributor

This is a follow up to the initial work done in PR #9463, in which the --coverage option was added in configure in order to build instrumented C++ code.

This PR adds in a make coverage recipe (as well as a make clean_coverage to clean up). Using make in this way will build node, instrument the javascript code, run the tests (as per make test) and give the coverage results. When used in conjunction with configure --coverage both C++ and Javascript coverage results will be available - and when used on its own, just Javascript.

Aside from giving a one-line coverage results, i.e -

Javascript coverage %: 90.16
C++ coverage %: 89.1

We also produce html reports (under the coverage directory) so a developer can drill down and inspect what changes their code has made.

This is very heavily based on the work @addaleax did in her coverage project https://github.com/addaleax/node-core-coverage as well as the overnight coverage reports @mhdawson created the CI job for here https://coverage.nodejs.org

ToDo:

  1. Generating the C++ coverage reports relies on pulling in the gcovr repository and then patching a file (using a patch from the testing work ground repo - and as per @addaleax original code) obviously it would more desirable not to patch anything, so next action is understand what needs to be done so this patch is no longer required.

  2. It may also be useful to give the option of holding multiple sets of results within the coverage directory, so a developer can produce a 'baseline' coverage results, and then compare this with any changes they might make.

FYI @Fishrock123 @gibfahn

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. lib / src Issues and PRs related to general changes in the lib or src directory. lts-watch-v6.x labels Jan 17, 2017
process.reallyExit = function(code) {
writeCoverage();
reallyReallyExit(code);
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, not 100% sold on this.

Is process.really a JS function? If so, can we put this hook into it directly somehow?

Also, could you point to a test that hits this but not the exit event?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

process.reallyExit is a C++ function.

Also, could you point to a test that hits this but not the exit event?

I put this in there to try to be really sure that writeCoverage() is called, especially since we have code that tests process.exit() itself… I can try to run coverage locally later without this, and see if there’s any difference

}

function setup() {
var reallyReallyExit = process.reallyExit;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const I suppose

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CurryKitten nit still

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it to a const

Makefile Outdated
# Build and test with code coverage reporting. Leave the lib directory instrumented for any additional runs the user may want to make
# For C++ coverage reporting, this needs to be run in conjunction with configure --coverage. html coverage reports will be
# created under ./coverage
coverage: all
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we separate JSCov and C++Cov somehow? Or at least, also have a separate JS cov make rule? Would be a much lower barrier to entry IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. Yes, can do - the C instrumentation is set in node.gyp from the configure option, but if it's a desirable option the coverage testing/reporting can be separated out into c/js make rules. My assumption would be to still have the default make coverage target run both

@Fishrock123 Fishrock123 self-assigned this Jan 17, 2017
@gibfahn
Copy link
Member

gibfahn commented Jan 17, 2017

cc/ @edsadr: I think I saw you asking about running the coverage manually.

also cc/ @addaleax

@mscdex mscdex added the test Issues and PRs related to the tests. label Jan 17, 2017
@edsadr
Copy link
Member

edsadr commented Jan 18, 2017

thanks for the heads up @gibfahn, looking forward for this one to land, amazing job @CurryKitten 🎉

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for picking this up!

Makefile Outdated

# Build and test with code coverage reporting. Leave the lib directory instrumented for any additional runs the user may want to make
# For C++ coverage reporting, this needs to be run in conjunction with configure --coverage. html coverage reports will be
# created under ./coverage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you try to wrap these lines at 80 characters? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - done

Makefile Outdated
$(NODE) ./deps/npm install istanbul-merge
$(NODE) ./deps/npm install nyc
git clone --depth=10 --single-branch git://github.com/gcovr/gcovr.git
git clone https://github.com/nodejs/testing.git
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be a shallow clone, too (--depth=10 or even less), I guess. It doesn’t make a difference right now but who knows what that repository might hold in the future…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have changed both clones to use a depth of 1, hopefully the clone of from the testing workgroup can be removed later. As you say, the gcovr code could change, in which case it might be wise to clone based on a specific commit, or do something else entirely

Makefile Outdated
$(MAKE)
$(MAKE) build-addons
$(MAKE) cctest
-$(PYTHON) tools/test.py --mode=release -J addons doctool inspector known_issues message pseudo-tty parallel sequential
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be its own make target, like test-ci etc. are?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, there are some tests that don’t work with coverage … it would probably make sense to add the skip in test/parallel/test-fs-sync-fd-leak.js, too, and drop message from the list of test directories?

Otherwise this breaks the coverage file merging for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - intention is to split into coverage-build, coverage-test and then coverage-with-test that runs both of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how I managed to fail to pick up your original changes in test/parallel/test-fs-sync-fd-leak.js making the appropriate changes, and dropping message from the list of test directories

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've put the skip into test/parallel/test-fs-sync-fd-leak.js and also in test/parallel/test-repl-tab-complete.js which I also found was failing when using coverage

Makefile Outdated
-rm -f out/Release/obj.target/node/src/*.gcda
-rm -f out/Release/obj.target/node/src/tracing/*.gcda
-rm -f out/Release/obj.target/node/src/*.gcno
-rm -f out/Release/obj.target/node/src/tracing*.gcno
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to user $(BUILDTYPE) for all the Release instances?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good call - done

Makefile Outdated
@@ -113,6 +113,47 @@ distclean:

check: test

# Remove files generated by running coverage, put the un-intstrumented lib back in place
clean_coverage:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This can be clean-coverage, all other targets in this file also use hyphens

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done

Makefile Outdated
$(NODE) ./node_modules/.bin/nyc instrument lib_/ lib/
$(MAKE)
$(MAKE) build-addons
$(MAKE) cctest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably run the subset of clean_coverage that removes all files containing coverage information before the actual tests are started

Makefile Outdated
(cd gcovr && patch -p1 < "$(CURDIR)/testing/coverage/gcovr-patches.diff")
rm -rf testing
mv lib lib_
$(NODE) ./node_modules/.bin/nyc instrument lib_/ lib/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be tricky to get everything wired up just right, but we can probably call nyc instrument from tools/js2c.py during the compilation process, so that there are no temporary files involved… that would avoid all the directory moving

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I was testing, I found that I needed both an instrumented copy of lib as well as the untouched version, so that when the html reports got generated, they picked up the non-instrumented copy of the source... which makes for easier reading. It's entirely possible that I may have this wrong though.

Copy link
Member

@addaleax addaleax Jan 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant was adding the instrumentation via nyc on the fly, so that we don’t need to replace anything in lib/; doing that is definitely a bit tricky, I can see if I can get something together and PR against your branch here

};

process.on('exit', writeCoverage);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth running the contents of setup() only conditionally, e.g. when global.__coverage__ is set (I think that test should work)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've put the check for global.__coverage__ into lib/internal/bootstrap_node.js so the require for internal/process/write-coverage isn't called unless coverage is being used

Makefile Outdated
$(NODE) ./deps/npm install nyc
git clone --depth=10 --single-branch git://github.com/gcovr/gcovr.git
git clone https://github.com/nodejs/testing.git
(cd gcovr && patch -p1 < "$(CURDIR)/testing/coverage/gcovr-patches.diff")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

… it might be time that one of us actually tries to find out why those patches are (or were) necessary and, if it makes sense, try to submit them upstream 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes :) ... and I'm happy to take that on. I took a quick look through gcovr, which essentially only revealed that I'd need to invest more time to take a proper look.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and I'm happy to take that on. I took a quick look through gcovr, which essentially only revealed that I'd need to invest more time to take a proper look.

Right, that’s how far I got, too ;)

process.reallyExit = function(code) {
writeCoverage();
reallyReallyExit(code);
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

process.reallyExit is a C++ function.

Also, could you point to a test that hits this but not the exit event?

I put this in there to try to be really sure that writeCoverage() is called, especially since we have code that tests process.exit() itself… I can try to run coverage locally later without this, and see if there’s any difference

Makefile Outdated
@@ -113,6 +113,69 @@ distclean:

check: test

# Remove files generated by running coverage, put the un-intstrumented lib back
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

intstrumented->intstrumented

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... or even instrumented ? I guess this is what comes from years of muscle memory used to typing int repeatedly

Makefile Outdated
@@ -113,6 +113,69 @@ distclean:

check: test

# Remove files generated by running coverage, put the un-intstrumented lib back
# in place
clean-coverage:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clean-coverage->coverage-clean for consistency with coverage-* (and the other clean targets).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Makefile Outdated
# For C++ coverage reporting, this needs to be run in conjunction with configure
# --coverage. html coverage reports will be created under ./coverage

coverage-with-test: coverage-build coverage-test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the standard thing people will want to run? If so maybe just call it coverage: (make lint, make test, make coverage).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - I would imagine that most people would want to run through the whole build & test with coverage on - so have changed to coverage

Makefile Outdated
# Build and test with code coverage reporting. Leave the lib directory
# instrumented for any additional runs the user may want to make.
# For C++ coverage reporting, this needs to be run in conjunction with configure
# --coverage. html coverage reports will be created under ./coverage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: ./coverage->coverage/ to make it clear that it's a directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, done

Makefile Outdated
@@ -758,4 +821,5 @@ endif
bench-all bench bench-misc bench-array bench-buffer bench-net \
bench-http bench-fs bench-tls cctest run-ci test-v8 test-v8-intl \
test-v8-benchmarks test-v8-all v8 lint-ci bench-ci jslint-ci doc-only \
$(TARBALL)-headers test-ci test-ci-native test-ci-js build-ci
$(TARBALL)-headers test-ci test-ci-native test-ci-js build-ci \
clean-coverage coverage-build coverage-test coverage-with-test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clean-coverage coverage-build coverage-test coverage-with-test->
coverage-clean coverage-build coverage-test coverage
as above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, done

Makefile Outdated
coverage: coverage-build coverage-test

coverage-build: all
mkdir node_modules
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this mkdir -p so it doesn’t fail when node_modules is already there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, added that

Makefile Outdated
-rm -f out/$(BUILDTYPE)/obj.target/node/src/*.gcda
-rm -f out/$(BUILDTYPE)/obj.target/node/src/tracing/*.gcda
-rm -f out/$(BUILDTYPE)/obj.target/node/src/*.gcno
-rm -f out/$(BUILDTYPE)/obj.target/node/src/tracing*.gcno
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we merge these with the corresponding lines in coverage-clean? Also, when invoked with -f rm should not fail for missing operands, so you can probably drop the - prefix

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I can do - although I noted that other targets such as clean and distclean also used the combination of the rm -f/-rf in combination with the - prefix, so is it best to keep the consistency or remove the leading -?

Makefile Outdated
-$(PYTHON) tools/test.py --mode=release -J addons doctool inspector \
known_issues pseudo-tty parallel sequential
mv lib lib__
mv lib_ lib
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, something like addaleax/node@85a9a95 might help us get rid of the directory-moving but right now it’s sloooow… nyc instrument has quite a bit of startup overhead :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks like a major step forwards. Does it need an existing node binary at the moment though - or different parameters to build. When I tried out the build from your repo it failed in out/Release/obj/gen/node_natives.h because it could find the node command to run node node_modules/.bin/nyc instrument lib/internal/bootstrap_node.js

Makefile Outdated

coverage-build: all
mkdir node_modules
$(NODE) ./deps/npm install istanbul-merge
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the installation and cloning(which require Internet access) should be done only when they are not already downloaded, or needs update. Probably testing the existence of these folders first would be better? An update can be signalled with a change to a file or something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem like a good idea to allow it to work off-line

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhdawson Are you suggesting that we check nyc into the repo? I’m not sure that’s a good idea given its size ;)

Copy link
Member

@mhdawson mhdawson Jan 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax, no just the idea that if its already downloaded, that you could continue to run make coverage without additional downloads being required. So check if you have already downloaded what's necessary so that the 2nd run an on don't necessarily need that.

I also think that is something we can break out into a follow on PR so that people can start using the make coverage target sooner than later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we could at least get it in first, the additional download thing can be addressed in another PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a change here - so we make assumptions that the modules/git clones exist based on the associated directories being present (and don't attempt to download again). Also check if gcovr has already been patched before patching (or not)

@mhdawson
Copy link
Member

Overall its looking good, once the number of comments/changes settle down I'll try it out in the CI job to validate we can use it there as well as by hand.

@Fishrock123
Copy link
Contributor

Going to bring up #10856 (comment) again:

Can we separate JSCov and C++Cov somehow? Or at least, also have a separate JS cov make rule? Would be a much lower barrier to entry IMO.

@mhdawson
Copy link
Member

@Fishrock123 is separating them out into 2 targets something you think we need to do before merging this or can it be done as a follow on ? I'm thinking having something people can use now would be useful (assuming it would take more than a couple of days for @CurryKitten to be able to break it out into the 2 targets)

@gibfahn
Copy link
Member

gibfahn commented Jan 26, 2017

@gibfahn
Copy link
Member

gibfahn commented Jan 26, 2017

I'm definitely +1 on this landing, even if it's not quite ideal, so that people can start to use it and iterate on it.

@jasnell
Copy link
Member

jasnell commented Jan 26, 2017

Definitely going to weigh in to support this but I'm not going to do a review and sign off given that it's not my strongest area. Really love to see this work tho.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a while to figure out why, but trying to get this to work in the CI seemed to fail at different points. I think the problem is related to using -j X where X is the number of cpus on the machine. It looks like there may be dependency problems where one step may try to start running before the earlier steps are complete and then the overall job fails. Throttling it down to 1 thread (no -j) it seems to run the steps as expected (although I still got an error later on but I'll have to re-run as that seemed to be complaining about a truncated file which might have just been a one off)

Can you investigate/fix to make sure that it works reliably doing

./configure --coverage
make coverage-clean
NODE_TEST_DIR=${HOME}/node-tmp PYTHON=python FLAKY_TESTS=$FLAKY_TESTS_MODE make coverage -j8

Makefile Outdated
$(MAKE)

coverage-test:
$(MAKE) build-addons
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this just run the test-ci target and then add what is necessary as opposed to defining the content separately ? Thinking that will make sure they stay in sync.

@mhdawson
Copy link
Member

So I got a run that went through to displaying the coverage numbers and they were reported as:

JS Coverage: 89.8 %
C++ Coverage: 88.1 %

Which is lower than the current job reports. Not sure if this is because what we are running different versus the run-ci target, the crashed test, I'm running off CurryKittens branch which is older or something else but we'll need to make sure this version is producing the same results as being shown on coverage.nodejs.org or at least understand why its different.

@Fishrock123
Copy link
Contributor

Applying: Initial files for js coverage
/Users/Jeremiah/Documents/node/.git/rebase-apply/patch:104: trailing whitespace.
  return leaked.filter((varname) => !/^__cov/.test(varname));
warning: 1 line adds whitespace errors.
Applying: Makefile change to allow -coverage option
/Users/Jeremiah/Documents/node/.git/rebase-apply/patch:30: trailing whitespace.
	$(NODE) ./node_modules/.bin/nyc instrument lib_/ lib/
/Users/Jeremiah/Documents/node/.git/rebase-apply/patch:35: trailing whitespace.
	mv lib lib__
/Users/Jeremiah/Documents/node/.git/rebase-apply/patch:61: trailing whitespace.
    return leaked.filter((varname) => !/^(cov_|__cov)/.test(varname));
warning: 3 lines add whitespace errors.
Applying: Fix rogue spaces in Makefile
Applying: Patch gcovr to provide C++ coverage
/Users/Jeremiah/Documents/node/.git/rebase-apply/patch:13: trailing whitespace.
# Remove files generated by running coverage, put the un-intstrumented lib back in place
/Users/Jeremiah/Documents/node/.git/rebase-apply/patch:27: trailing whitespace.
# For C++ coverage reporting, this needs to be run in conjunction with configure --coverage.  html coverage reports will be
warning: 2 lines add whitespace errors.
Applying: Patch gcovr for C++ coverage
Applying: Correct variable reference in Makefile
Applying: Fix formatting issues in Makefile
/Users/Jeremiah/Documents/node/.git/rebase-apply/patch:15: trailing whitespace.
# Remove files generated by running coverage, put the un-intstrumented lib back
/Users/Jeremiah/Documents/node/.git/rebase-apply/patch:16: trailing whitespace.
# in place
/Users/Jeremiah/Documents/node/.git/rebase-apply/patch:35: trailing whitespace.
# Build and test with code coverage reporting.  Leave the lib directory
/Users/Jeremiah/Documents/node/.git/rebase-apply/patch:56: space before tab in indent.
        	.cov_tmp/libcov.json 'out/Release/.coverage/coverage-*.json'
warning: 4 lines add whitespace errors.
Applying: Shallow clone the required external git repos
Applying: Split coverage into separate build/test jobs
Applying: Changes as per comments from reviews in PR
/Users/Jeremiah/Documents/node/.git/rebase-apply/patch:18: trailing whitespace.
# Remove files generated by running coverage, put the non-instrumented lib back
warning: 1 line adds whitespace errors.
Applying: Allow coverage to work offline - if the prereqs exist

Looks like there are some whitespace errors, could you please set git config --global --add core.whitespace fix, or at least set it for the Node.js repo? :D

Just saw this while trying it out. :)

@Fishrock123
Copy link
Contributor

Hmmm, no JavaScript %? (This was without configure --coverage.)

Seems to have a correct coverage/index.html file though.

rm -rf out/Release/.coverage
rm -rf .cov_tmp
rm -f out/Release/obj.target/node/src/*.gcda
rm -f out/Release/obj.target/node/src/tracing/*.gcda
rm -f out/Release/obj.target/node/src/*.gcno
rm -f out/Release/obj.target/node/src/tracing*.gcno
/usr/bin/python tools/test.py --mode=release -J addons doctool inspector \
		known_issues pseudo-tty parallel sequential
[03:42|% 100|+ 1326|-   0]: Done
mv lib lib__
mv lib_ lib
mkdir -p coverage .cov_tmp
./node ./node_modules/.bin/istanbul-merge --out \
        	.cov_tmp/libcov.json 'out/Release/.coverage/coverage-*.json'
(cd lib && ../node ../node_modules/.bin/nyc report \
		--temp-directory "/Users/Jeremiah/Documents/node/.cov_tmp" -r html \
		--report-dir "../coverage")
(cd out && "../gcovr/scripts/gcovr" --gcov-exclude='.*deps' \
		--gcov-exclude='.*usr' -v -r Release/obj.target/node \
		--html --html-detail -o ../coverage/cxxcoverage.html)
Scanning directory Release/obj.target/node for gcda/gcno files...
Found 0 files (and will process 0)
Gathered coveraged data for 0 files
mv lib lib_
mv lib__ lib
-n Javascript coverage %:

-n C++ coverage %:
0.0

@CurryKitten
Copy link
Contributor Author

@Fishrock123 core.whitespace fix has been added to my git settings.

As to the lack of reported JS coverage in your example (when the html file looks good) is I think down to the fact that grep -o is sometimes not supported - depending on your environment/os of course.

To make things more portable, I'll change the offending grep to use sed. So in the JS case from

@grep -B1 Lines coverage/index.html | head -n1 | grep -o '[0-9\.]*'

to

@grep -B1 Lines coverage/index.html | head -n1 \
               | sed 's/<[^>]*>//g'| sed 's/ //g'

With something similar for gathering the C coverage summary.

@mhdawson
Copy link
Member

@CurryKitten any progress on looking at the -J issues? Let me know when I should test out in the CI again.

@CurryKitten
Copy link
Contributor Author

@mhdawson Yes - I believe these have been fixed (although not pushed into my fork yet). I was looking at the change to use $(MAKE) test-ci instead of build-addons/cctest and then running test.py against a set of suites. From a suggestion from @addaleax I'd removed the message test suite, but as test-ci brings this back in, there are a few failures coverage causes to work around.

@mhdawson
Copy link
Member

@CurryKitten I thought we were going to make it run the tests but accept failures ? I think the commit still turns off testing for 2 of the tests.

@mhdawson
Copy link
Member

So run with current version of this PR shows:
JS Coverage: 90.6 %
C++ Coverage: 90.4 %

But if I remove the 2 test exclusions it pops up to:

JS Coverage: 90.96 %
C++ Coverage: 90.5 %

Which is lower than reported in CI:

However, if I remove the parts that exclude the tests it pops back up to the current CI numbers:

JS Coverage: 90.96 %
C++ Coverage: 90.5 %

@mhdawson
Copy link
Member

mhdawson commented Feb 15, 2017

@CurryKitten created pull request against your repo to pull that change in: https://github.com/CurryKitten/node/pull/1

I think if we pull that in and then I can land after one more node CI run.

@mhdawson
Copy link
Member

At this point it would also be good if you squashed the commits down to 1

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the addition of the commit in my PR and one more Node CI run.

Makefile Outdated
-rm -f out/$(BUILDTYPE)/obj.target/node/src/*.gcno
-rm -f out/$(BUILDTYPE)/obj.target/node/src/tracing*.gcno
-$(PYTHON) tools/test.py --mode=release -J addons doctool inspector \
known_issues pseudo-tty parallel sequential
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the second problem I was seeing the CI may be related to the -J, that seems to often cause problem by spawning as many parallel processes as possible. The error I saw was:

=== release test-https-close ===
Path: parallel/test-https-close
Command: out/Release/node /home/iojs/build/workspace/node-test-commit-linux-coverage-mdawson/nodes/benchmark/test/parallel/test-https-close.js
--- CRASHED (Signal: 11) ---

It really should inherit the parallelism from whatever the command line was, like the regular test job does

Copy link
Member

@gibfahn gibfahn Feb 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhdawson -J should use the $JOBS environment variable or the number of CPUs, it definitely shouldn't be spawning as many processes as possible.

Source here: https://github.com/nodejs/node/blob/master/tools/test.py#L1429-L1433

@CurryKitten
Copy link
Contributor Author

@mhdawson I committed the changes to re-enable the disabled tests and squashed the commits down.

@@ -1,5 +1,5 @@
'use strict';
require('../common');
const common = require('../common');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like this is necessary?

Can you run make lint?

@mhdawson
Copy link
Member

I'm not in a place I can run make lint right now but will run CI: https://ci.nodejs.org/job/node-test-pull-request/6449/

@mhdawson
Copy link
Member

yup lint failure in CI run.

actually failure

./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules
benchmark lib test tools

/home/mhdawson/newpull10/io.js/test/parallel/test-fs-sync-fd-leak.js
2:7 error 'common' is assigned a value but never used no-unused-vars

â 1 problem (1 error, 0 warnings)

@CurryKitten can you fix that up and make sure to run make test, once pushed I'll run the CI one more time and hopefully be able to land.

Makefile Outdated
@grep -A3 Lines coverage/cxxcoverage.html | grep style \
| sed 's/<[^>]*>//g'| sed 's/ //g'


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too much whitespace, need to remove two of these

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@CurryKitten
Copy link
Contributor Author

@mhdawson Changes made, make test and lint run cleanly

@mhdawson
Copy link
Member

Ok one more CI run: https://ci.nodejs.org/job/node-test-pull-request/6470/

@mhdawson
Copy link
Member

Ok CI run is green, going to land

@mhdawson
Copy link
Member

Landed as f8ee197

@mhdawson
Copy link
Member

Next step is to update coverage CI jobs

mhdawson pushed a commit that referenced this pull request Feb 17, 2017
PR-URL: #10856
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member

CI jobs updated, looks like they ran ok, will need to to final check once results are mirrored to coverage.nodejs.org.

Will put together blog to raise awareness and point out a few of the key things to know about using it.

@Fishrock123
Copy link
Contributor

Closing this as it was merged in f8ee197

@mhdawson can you make a new thread to track additional work?

@mhdawson
Copy link
Member

addaleax pushed a commit that referenced this pull request Feb 22, 2017
PR-URL: #10856
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@italoacasas italoacasas mentioned this pull request Feb 25, 2017
@jasnell
Copy link
Member

jasnell commented Mar 7, 2017

requires a backport PR to land on v6 or v4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.