-
Notifications
You must be signed in to change notification settings - Fork 19
doc: initial version of doc for coverage generation #37
Conversation
|
||
4) Build/test as per normal build/test job. This is currently: | ||
|
||
NODE_TEST_DIR=${HOME}/node-tmp PYTHON=python FLAKY_TESTS=$FLAKY_TESTS_MODE make run-ci -j $(getconf _NPROCESSORS_ONLN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might benefit from being enclosed in backticks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, will do that once I see if there are any other comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like everything after this is italics.
|
||
Generation/publication of the code coverage results consists of the following: | ||
|
||
* nightly scheduled job - We have a job in jenkins which is scheduled to run at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe capitalize "nightly"
[node-test-commit-linux-coverage](https://ci.nodejs.org/view/All/job/node-test-commit-linux-coverage/). | ||
* At the end of the scheduled job it rsnyc's the generated data to the | ||
benchmarking data machine. We do this so that once the job is complete | ||
the data is in a place where we know we can pull it from, and that pulling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double space after the comma.
updated once that is complete: | ||
``` | ||
if [ ! -d node-core-coverage ]; then | ||
git clone --depth=10 --single-branch git://github.com/addaleax/node-core-coverage.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're doing official things with it, perhaps @addaleax would be OK with moving the repo to the foundation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps @addaleax would be OK with moving the repo to the foundation.
Always! Given that Michael asked me to PR the files against the nodejs/testing repo, I assumed that was what he had in mind for a long-term solution (which seems perfectly fine to me). But if it’s decided that my repo should serve as a basis for anything I’d have no problems with moving it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @addaleax mentions I think just moving the files into the nodejs/testing repo is what makes sense to me.
them in the right place to be served at coverage.nodejs.org. The key | ||
required to do this is already in place in order to support the similar process | ||
for benchmarking.nodejs.org | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove the extra lines.
I wonder if the split should be: testing repo: Documentation on using/reading the code coverage, caveats about its limitations, etc. build repo: Documentation on how the code coverage tool actually works, how to deploy it to our infrastructure, etc. If I understand correctly, all the Jenkins/etc. docs are over in the build repo, and I would imagine we don't want to fragment things. |
@Trott I can see some of the reasoning for splitting things across the repos, although I also think having information in one place makes sense as well. In terms of the benchmarking runs/chart generation its documented in the benchmarking WG. I'd suggest we don't delay landing this, but then raise it with the build WG for discussion on whether moving the build specific parts over to the build repo would make sense. |
Land away! This repo and corresponding WG are not very active. (The WG is now mostly just a collection of people that care enough about tests that they don't mind being looped in on oddball test questions a few times a week.) So putting things here is kind of putting them in an out-of-the-way place, in a way. But I don't object. And if you make the repo more active, then awesome. |
LGTM addressing Colin comments.
I agree. It can be moved later if a better place is chosen. |
additions: | ||
|
||
|
||
1) Checkout of the scripts used to generate the coverage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use proper markdown for lists (1.
instead of 1)
).
fi | ||
``` | ||
|
||
2) get a copy of gcov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get a copy of gcov:
fi | ||
``` | ||
|
||
3) install npms that we use to instrument node.js and generate javascript |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
install -> Install
node.js -> Node.js (throughout the doc)
javascript -> JavaScript (throughout the doc)
fi | ||
``` | ||
|
||
3) install npms that we use to instrument node.js and generate javascript |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by install npms? Should that be npm modules?
tree (patches.diff). We will work to build those changes into the Makefile | ||
so that there are additional targets that can be used for code coverage | ||
runs and that patching the source is no longer required. This will | ||
reduce the likelyhood/frequency of conflicts causing the code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
likelyhood -> likelihood
but modified for that test failures don't stop the rest of the process as the | ||
instrumentation seems to have introduced a couple of failures. | ||
|
||
5) gather coverage and push to the benchmarking data machine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gather -> Gather, and add a colon at the end of the line please.
@cjihrig believe I have address latest comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, thanks for the effort you’re putting into getting this to work!
|
||
|
||
1. Checkout of the scripts used to generate the coverage | ||
These will be moved to github./com/testing/coverage and the job |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: ./com
… maybe use the full URL to turn this into a link? (also: this should probably be nodejs/testing
?)
a while. | ||
|
||
This doc captures the infrastructure in place to support the generation | ||
of the coverage information published to coverage.nodejs.org. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe add https://
before the domain?
* Nightly scheduled job - We have a job in jenkins which is scheduled to run at | ||
11 EST each night. | ||
[node-test-commit-linux-coverage](https://ci.nodejs.org/view/All/job/node-test-commit-linux-coverage/). | ||
* At the end of the scheduled job it rsnyc's the generated data to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: rsnyc
updated once that is complete: | ||
``` | ||
if [ ! -d node-core-coverage ]; then | ||
git clone --depth=10 --single-branch git://github.com/addaleax/node-core-coverage.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This document is going to be updated too once the files have been checked into nodejs/testing
or somewhere else under the org… correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes will update as we adjust going forward.
benchmarking machine | ||
[iojs-softlayer-benchmark](https://ci.nodejs.org/computer/iojs-softlayer-benchmark/), | ||
have installed the key there, and have added an entry in | ||
the .ssh/config file for the iojs user so that connections to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I’d enclose .ssh/config
in backticks
PR-URL: #37 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
landed as: 766a90e |
PR-URL: nodejs/testing#37 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This is my initial doc for the process of generating nightly coverage results. Not all the steps are in place yet but the job is running nightly and data is being pushed to one of my personal machines so results can be viewed at http://nodejs.devrus.com:8080/ until we get them onto coverage.nodejs.org.
There will need to be updates as we move the pieces needed into github/nodejs/testing/coverage as opposed to pulling from @addaleax's repo (and of course as we improve the flow).
The steps are as documented by @addaleax in earlier work as discussed in #36 with a few small tweaks to fit in to our standard build structure. I also tweaked the index generation to fit with the nodejs.org website.
Currently there is no pruning of the data so it will grow each day, but I'd rather keep more data until the amount becomes an issue.
I also have not covered backups, but that should be a straight forward extension of what we already do for the benchmarking data backups.