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

Add code coverage reporting #1866

Merged
merged 4 commits into from
Jan 23, 2019
Merged

Add code coverage reporting #1866

merged 4 commits into from
Jan 23, 2019

Conversation

dleve123
Copy link
Contributor

@dleve123 dleve123 commented Jan 18, 2019

Code Coverage Info in CodeCov

screen shot 2019-01-22 at 6 26 37 pm

Anyone within the Artsy org should be able to sign into Codecov via Github-auth to get more granular coverage statistics: https://codecov.io/gh/artsy/positron

Code Coverage Into in Console Output

screen shot 2019-01-23 at 1 02 46 pm


Supersedes #1854 (wanted to rename branch)

https://artsyproduct.atlassian.net/browse/PLATFORM-1136

Problem:

One part of the product health matrix story spike is code
coverage for each service. However, there is no pattern at Artsy for
measuring code coverage for Javascript applications.

While there are existing tools out there for tracking and reporting code
coverage, such as istanbul.js, coveralls,
and codecov, the complexities:

  1. A multi-runner test suite (jest and mocha), and
  2. A Circle CI -> Docker -> Docker test runtime

have resulted in a tough problem to solution around.

Solution:

After spending a lot of time trying to get Coveralls instrumentation to
work, we pivoted to using Codecov. At a high level, this commit does the
following:

  1. Updates our calls to jest and mocha to write code coverage
    information to a .nyc_output directory (default for nyc/istanbul)

  2. Manipulates the jest coverage information to be compatible with
    mocha coverage information via a JS script. See motivating GH
    issue
    .

  3. Merges the jest and mocha coverage information together using the
    nyc CLI

  4. Introduces a bash executable provided by codecov to instrument the
    merged information to codecov

Co-authored-by: Kieran Gillen [email protected]

https://artsyproduct.atlassian.net/browse/PLATFORM-1136

Problem:

One part of the product health matrix story [spike][spike] is code
coverage for each service. However, there is no pattern at Artsy for
measuring code coverage for Javascript applications.

While there are existing tools out there for tracking and reporting code
coverage, such as [istanbul.js][istanbul-js], [coveralls][coveralls],
and [codecov][codecov], the complexities:

1. A multi-runner test suite (jest and mocha), and
2. A Circle CI -> Docker -> Docker test runtime

have resulted in a tough problem to solution around.

Solution:

After spending a lot of time trying to get Coveralls instrumentation to
work, we pivoted to using Codecov. At a high level, this commit does the
following:

1. Updates our calls to `jest` and `mocha` to write code coverage
information to a `.nyc_output` directory (default for nyc/istanbul)

2. Manipulates the `jest` coverage information to be compatible with
`mocha` coverage information via a JS script. See [motivating GH
issue][gh-issue].

3. Merges the `jest` and `mocha` coverage information together using the
`nyc` CLI

4. Introduces a bash executable provided by codecov to instrument the
merged information to codecov

Co-authored-by: Kieran Gillen <[email protected]>

[spike]:https://artsyproduct.atlassian.net/browse/PLATFORM-1098
[istanbul-js]:https://github.com/istanbuljs/nyc
[coveralls]:https://coveralls.io/
[codecov]:https://codecov.io/gh/artsy/positron/
[gh-issue]:jestjs/jest#2418
* Add BRANCH_NAME to .env.test so that it's whitelisted for Docker
* Pass through CIRCLE_BRANCH to `hokusai test`
* Extend expression to `codecov` to take a branch name
@damassi
Copy link
Member

damassi commented Jan 22, 2019

@dleve123 - can we add a screenshot of example output from the CLI within the top description?

const path = require('path')

const normalizeJestCoverage = (obj) => {
const result = { ...obj };
Copy link
Member

Choose a reason for hiding this comment

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

Why this line instead of renaming the obj arg result?

fs.copyFile(
'./coverage/coverage-final.json',
'./.nyc_output/coverage-final.json',
(error) => { if (error) console.log(error) }
Copy link
Member

@damassi damassi Jan 22, 2019

Choose a reason for hiding this comment

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

This looks super readable (not suggesting a change) but could also do

error => error && console.log(error) 

Also, by the way this is formatted I can tell your editor is missing a Prettier integration :) which we use for all of our JS repos based on configuration located here.

@dleve123
Copy link
Contributor Author

@damassi Is Prettier compliance enforced during CI? I think style checking would be a bit more robust if we asserted upon a certain style during CI, instead of assuming that everyone use an editor that is integrated with Prettier.

@dleve123 dleve123 merged commit 6963ebf into master Jan 23, 2019
@damassi
Copy link
Member

damassi commented Jan 23, 2019

On all of our JS repos we typically run things like prettier during a pre-commit hook which is defined here -- not too sure why this didn't run?

@damassi damassi deleted the add-test-coverage branch January 23, 2019 18:57
@dleve123
Copy link
Contributor Author

It's possible that I ran a --no-verify when committing at times -- that probably explains it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants