Skip to content
This repository was archived by the owner on May 13, 2019. It is now read-only.

doc: initial version of doc for coverage generation #37

Closed
wants to merge 5 commits into from

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented Oct 12, 2016

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.

@mhdawson mhdawson mentioned this pull request Oct 12, 2016
11 tasks

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)
Copy link
Member

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?

Copy link
Member Author

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.

Copy link

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
Copy link

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
Copy link

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
Copy link

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.

Copy link
Member

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.

Copy link
Member Author

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

Copy link

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.

@Trott
Copy link
Member

Trott commented Oct 14, 2016

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.

@mhdawson
Copy link
Member Author

@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.

@Trott
Copy link
Member

Trott commented Oct 19, 2016

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.

@santigimeno
Copy link
Member

LGTM addressing Colin comments.

Land away!

I agree. It can be moved later if a better place is chosen.

@mhdawson
Copy link
Member Author

Ok, all comments addressed. If I can get a second LTGM I'll go ahead and land. @addaleax. @cjihrig ?

additions:


1) Checkout of the scripts used to generate the coverage
Copy link

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
Copy link

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
Copy link

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
Copy link

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
Copy link

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
Copy link

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.

@mhdawson
Copy link
Member Author

@cjihrig believe I have address latest comments.

Copy link

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

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.

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
Copy link
Member

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.
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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?

Copy link
Member Author

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
Copy link
Member

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

mhdawson added a commit that referenced this pull request Oct 20, 2016
PR-URL: #37
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@mhdawson
Copy link
Member Author

landed as: 766a90e

@mhdawson mhdawson closed this Oct 20, 2016
maclover7 pushed a commit to maclover7/build that referenced this pull request Dec 31, 2017
PR-URL: nodejs/testing#37
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants