Add documentation for failing builds on unmet thresholds by charlesdemers · Pull Request #173 · ember-cli-code-coverage/ember-cli-code-coverage

charlesdemers · 2018-04-11T16:56:17Z

As discussed in #23 I added the instructions to setup nyc to fail the build when coverage thresholds are not met.

Feel free to make corrections, although I made my best, english is not my native language so… :)

cvx · 2018-04-13T10:39:06Z

Could you share your config/coverage.js?

I've tried running nyc check-coverage when using master ember-cli-code-coverage and .nycrc from this PR but it resulted in an error:

Error: Invalid file coverage object, missing keys, found:lines,statements,functions,branches
    at assertValidObject (/app/node_modules/nyc/node_modules/istanbul-lib-coverage/lib/file.js:129:15)
    at new FileCoverage (/app/node_modules/nyc/node_modules/istanbul-lib-coverage/lib/file.js:164:5)
    at /app/node_modules/nyc/node_modules/istanbul-lib-coverage/lib/coverage-map.js:17:23
    at Array.forEach (<anonymous>)
    at loadMap (/app/node_modules/nyc/node_modules/istanbul-lib-coverage/lib/coverage-map.js:12:25)
    at new CoverageMap (/app/node_modules/nyc/node_modules/istanbul-lib-coverage/lib/coverage-map.js:34:21)
    at CoverageMap.merge (/app/node_modules/nyc/node_modules/istanbul-lib-coverage/lib/coverage-map.js:49:17)
    at /app/node_modules/nyc/index.js:429:9
    at Array.forEach (<anonymous>)
    at NYC._getCoverageMapFromAllCoverageFiles (/app/node_modules/nyc/index.js:428:22)

charlesdemers · 2018-04-13T11:45:00Z

@cvx The coverage.js file doesn’t contain much:

/* eslint-env node */

module.exports = {
  useBabelInstrumenter: true
};

I think you’re getting an error because you’re using the released version of ember-cli-code-coverage, am I right? The patch that updates babel-plugin-instanbul to output coverage files compatible with nyc isn’t released yet.

@kategengler @rwwagner90 I’m guessing you’ll make a release when this is merged?

cvx · 2018-04-13T12:18:08Z

I'm using the current master of ember-cli-code-coverage (because I wanted to check this feature out):

    "ember-cli-code-coverage": "kategengler/ember-cli-code-coverage#77b5b92",

Babel-plugin-istanbul is also in the latest version (4.1.6). Do you know which package specifically were those compatibility changes introduced in?

charlesdemers · 2018-04-13T12:34:31Z

I don’t know unfortunately... looking at their commit log there’s a bunch of updates for istanbul and nyc it’s hard to say which one changed the output format

RobbieTheWagner · 2018-04-13T13:32:20Z

I thought this PR was all we needed? #169 The description said it was.

@charlesdemers what version of ember-cli-code-coverage are you on?

charlesdemers · 2018-04-13T15:17:24Z

I'm still linking my package.json on the branch I used to create the pull request:

 "ember-cli-code-coverage": "github:charlesdemers/ember-cli-code-coverage#6961be288d5666e0c8eae4d1988bbfac3773b433"

RobbieTheWagner · 2018-04-13T15:35:51Z

@charlesdemers can you try using master and see if you encounter the same issue?

charlesdemers · 2018-04-13T17:14:13Z

I can reproduce the issue with master. The only difference is the code for the merge-coverage command was not re-added in my branch.

I won’t have much time until next week but I have a project which uses ember-exam, I’ll try and see if I can make it work without merge-coverage

RobbieTheWagner · 2018-04-13T19:34:53Z

Ah, I see. @adamjmcgrath perhaps you could give some insight?

adamjmcgrath · 2018-04-13T20:52:22Z

"ember-cli-code-coverage": "github:charlesdemers/ember-cli-code-coverage#6961be288d5666e0c8eae4d1988bbfac3773b433"

I was able to reproduce the issue with a new ember app and your branch

$ ember new foo
$ cd foo
$ ember install charlesdemers/ember-cli-code-coverage#6961be288d5666e0c8eae4d1988bbfac3773b433
$ npm install nyc
$ echo "module.exports = { reporters: ['json-summary'] }" > config/coverage.js
$ echo '{"temp-directory":"coverage"}' > .nycrc
$ COVERAGE=true ember t
$ nyc check-coverage
/Users/adam/dev/tmp/foo/node_modules/nyc/node_modules/yargs/yargs.js:1133
      else throw err
           ^

Error: Invalid file coverage object, missing keys, found:lines,statements,functions,branches
    at assertValidObject (/Users/adam/dev/tmp/foo/node_modules/nyc/node_modules/istanbul-lib-coverage/lib/file.js:129:15)
    at new FileCoverage (/Users/adam/dev/tmp/foo/node_modules/nyc/node_modules/istanbul-lib-coverage/lib/file.js:164:5)
    at /Users/adam/dev/tmp/foo/node_modules/nyc/node_modules/istanbul-lib-coverage/lib/coverage-map.js:17:23
    at Array.forEach (<anonymous>)
    at loadMap (/Users/adam/dev/tmp/foo/node_modules/nyc/node_modules/istanbul-lib-coverage/lib/coverage-map.js:12:25)
    at new CoverageMap (/Users/adam/dev/tmp/foo/node_modules/nyc/node_modules/istanbul-lib-coverage/lib/coverage-map.js:34:21)
    at CoverageMap.merge (/Users/adam/dev/tmp/foo/node_modules/nyc/node_modules/istanbul-lib-coverage/lib/coverage-map.js:49:17)
    at /Users/adam/dev/tmp/foo/node_modules/nyc/index.js:429:9
    at Array.forEach (<anonymous>)
    at NYC._getCoverageMapFromAllCoverageFiles (/Users/adam/dev/tmp/foo/node_modules/nyc/index.js:428:22)

I can only assume that the latest nyc (11.6.0) is not compatible with reports generated by the latest babel-plugin-istanbul(4.1.6)

$ npm list nyc
foo@0.0.0 /Users/adam/dev/tmp/foo
└── nyc@11.6.0 
$ npm list babel-plugin-istanbul
foo@0.0.0 /Users/adam/dev/tmp/foo
└─┬ ember-cli-code-coverage@1.0.0-beta.3 (github:charlesdemers/ember-cli-code-coverage#6961be288d5666e0c8eae4d1988bbfac3773b433)
  └── babel-plugin-istanbul@4.1.6

adamjmcgrath · 2018-04-13T20:56:40Z

I can only assume that the latest nyc (11.6.0) is not compatible with reports generated by the latest babel-plugin-istanbul(4.1.6)

Sorry, that should read "...with the reports generated by ember-cli-code-coverage" (babel-plugin-istanbul just does the instrumentation)

kategengler · 2018-04-13T21:43:05Z

I think there is a newer version of istanbul-api (which is what is used for the reports), that might prove more compatible?

adamjmcgrath · 2018-04-13T22:22:15Z

$ echo "module.exports = { reporters: ['json-summary'] }" > config/coverage.js

It turns out nyc doesn't like the json-summary report, so charlesdemers/ember-cli-code-coverage#6961be288d5666e0c8eae4d1988bbfac3773b433 works as long as you use the json report.

What's in master doesn't work with nyc because we're always adding json-summary (https://github.com/kategengler/ember-cli-code-coverage/blob/master/lib/attach-middleware.js#L37-L39) since we restored the merge-coverage command.

Not sure what to suggest, you could work around it by suggesting people do: "check-coverage": "rm coverage/coverage-summary.json && ./node_modules/.bin/nyc check-coverage"

adamjmcgrath · 2018-04-13T22:24:33Z

$ ember new foo
$ cd foo
$ ember install kategengler/ember-cli-code-coverage#master
$ npm install nyc
$ echo "module.exports = { reporters: ['json'] }" > config/coverage.js
$ echo '{"temp-directory":"coverage", "functions": 100}' > .nycrc
$ COVERAGE=true ember t
$ rm coverage/coverage-summary.json
$ nyc check-coverage
ERROR: Coverage for lines (0%) does not meet threshold (90%) for /Users/adam/dev/tmp/foo/app/app.js
ERROR: Coverage for lines (0%) does not meet threshold (90%) for /Users/adam/dev/tmp/foo/app/router.js
ERROR: Coverage for functions (0%) does not meet threshold (100%) for /Users/adam/dev/tmp/foo/app/router.js
...

RobbieTheWagner · 2018-04-14T20:18:43Z

@adamjmcgrath do we need that summary?

adamjmcgrath · 2018-04-17T06:21:49Z

Sure, would be nice if we could make this compatible with nyc - but I can't see an obvious fix for this issue

RobbieTheWagner · 2018-04-17T11:13:33Z

@adamjmcgrath what if we added the json reporter in first, before json-summary? Would nyc grab that output from the json reporter then, but we still retain the json-summary one for our use?

adamjmcgrath · 2018-04-17T12:49:51Z

Unfortunately not, nyc check-coverage assumes every json file in the coverage directory is a valid FileCoverage object. So as long as there's a json file in the directory that doesn't match this spec ({ "path/file.js": { 'statementMap': ... } }, ... }) it will error.

You'd need to make nyc check-coverage ignore coverage summary reports in addition to ignoring reports that aren't json.

RobbieTheWagner · 2018-04-17T13:24:25Z

@adamjmcgrath ah, I see. What about if we added another folder underneath coverage and moved some files around? It feels like there should be a way to get this to work without the need to delete the other files.

@kategengler any thoughts on how to handle this?

kategengler · 2018-04-17T13:27:08Z

I need to look into it a bit more, but since json-summary is a report type generated by istanbul, and nyc claims to support that report type (https://github.com/istanbuljs/nyc#running-reports), this should probably be fixed on the nyc side.

RobbieTheWagner · 2018-04-17T13:47:30Z

this should probably be fixed on the nyc side

Would someone want to open an issue to discuss? I can open one, but I don't know much about nyc, so it might be better coming from someone else.

kategengler · 2018-04-17T14:00:21Z

I also don't know anything about `nyc` but I will open the issue if nobody else wants to do so.

…

On Tue, Apr 17, 2018 at 9:47 AM, Robert Wagner ***@***.***> wrote: this should probably be fixed on the nyc side Would someone want to open an issue to discuss? I can open one, but I don't know much about nyc, so it might be better coming from someone else. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#173 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAbHOkKjnLgWAAxp1Ua3ZMtllf_TJvSiks5tpfJ2gaJpZM4TQZRR> .

RobbieTheWagner · 2018-04-17T14:10:37Z

@kategengler I can open it. Will do so now.

RobbieTheWagner · 2018-04-17T14:14:46Z

Issue filed istanbuljs/nyc#815. If anyone has extra things to add to it, please feel free to.

RobbieTheWagner · 2018-08-30T14:01:06Z

@adamjmcgrath @charlesdemers does this happen to "just work" yet? I know nyc has released some new versions.

charlesdemers · 2018-10-17T19:02:40Z

@rwwagner90 I confirm, it works with these versions in my current project at work 🎉

"ember-cli-code-coverage": "1.0.0-beta.6",
"nyc": "13.1.0"

charlesdemers · 2018-10-17T19:37:59Z

I spoke too quick, the issue is still present 😞

RobbieTheWagner · 2018-10-18T12:35:19Z

@charlesdemers any ideas what the issue is or how we could help figure out a fix?

charlesdemers · 2018-10-20T16:33:12Z

I tried my fork with the latest nyc version, it still works but the latest version of this addon (1.0.0-beta.6) doesn’t. The error is still the same:

Invalid file coverage object, missing keys, found:lines,statements,functions,branches

I really don’t know how to fix this on our end, other than remove the json-summary output since it seems to be the problematic one…

RobbieTheWagner · 2018-10-21T19:04:30Z

@charlesdemers what if we run nyc first, to get the output how it wants it to be, then call check-coverage?

JaKXz · 2019-01-05T23:22:57Z

README.md

+```json
+"scripts": {
+  "test": "COVERAGE=true ember test",
+  "check-coverage": "./node_modules/.bin/nyc check-coverage"


👋 Istanbul member here - how does ember produce the coverage result?

Does it do the instrumentation? In which case you would want to set instrument: false in your nyc config.

Finally, if it produces a coverage report, I think you would want this command to be nyc report --report-dir ./custom-report-dir, but I don't think you can have it actually fail based on the coverage check/watermarks if nyc is not doing the instrumenting. Correct me if I'm wrong @bcoe @coreyfarrell

I'm not sure the nyc report sub-command checks watermarks, I think it just generates reports. I do know that nyc checks watermarks, for example one of my projects vinyl-rollup uses instrument: false and coverage check works. If you add unreachable code to index.mjs it'll fail due to insufficient coverage, nyc exits with a non-zero code.

@JaKXz Thanks for chiming in!

The instrumentation is done using babel-plugin-istanbul at build time.

The reports are generated using istanbul-api (https://github.com/kategengler/ember-cli-code-coverage/blob/master/lib/attach-middleware.js#L35) in a middleware that receives the coverage data from browser-based tests.

It does sound like we'd need the nyc configuration suggested here: https://github.com/istanbuljs/nyc#use-with-babel-plugin-istanbul-for-babel-support

kategengler · 2019-01-06T02:44:39Z

@rwwagner90 The json-summary reporter is required because that is how we display the coverage results on the test reporter.

Leooo · 2020-09-28T11:40:28Z

Seeing the same error message:

Invalid file coverage object, missing keys, found:lines,statements,functions,branches

with the following versions:
ember-cli-code-coverage: 1.0.0
nyc: 15.1.0

running COVERAGE=true ember exam --split=8 --parallel followed by ./node_modules/.bin/nyc check-coverage

.nyrc:

{
  "all": true,
  "check-coverage": true,
  "per-file": true,
  "temp-directory": "coverage",
  "branches": 60,
  "functions": 60,
  "lines": 60,
  "statements": 60,
  "watermarks": {
    "branches": [70, 80],
    "functions": [70, 80],
    "lines": [70, 80],
    "statements": [70, 80]
  }
}

config/coverage.js:

module.exports = {
  reporters: ['lcov', 'html', 'cobertura'],
  parallel: true
};

PS: not clear following the above if babel-plugin-istanbul is needed, and if it is, what are the related settings / files to add

charlesdemers · 2020-09-28T11:52:46Z

The project seems to be under heavy refactoring and I don’t even use nyc anymore since it was not supported so I’m going to close this, feel free to create another issue to support checking thresholds

Leooo · 2020-09-28T12:02:50Z

ok :( anyone who has a better way to add coverage-based PR status checks, please feel free to share.

Add documentation for failing builds on unmet thresholds

b582ef6

cvx mentioned this pull request Apr 16, 2018

Add ember-cli-code-coverage. rust-lang/crates.io#1356

Merged

RobbieTheWagner mentioned this pull request Apr 17, 2018

Not working with json-summary format istanbuljs/nyc#815

Closed

JaKXz reviewed Jan 5, 2019

View reviewed changes

Turbo87 added the documentation label Aug 29, 2020

charlesdemers closed this Sep 28, 2020

Conversation

charlesdemers commented Apr 11, 2018

Uh oh!

cvx commented Apr 13, 2018

Uh oh!

charlesdemers commented Apr 13, 2018

Uh oh!

cvx commented Apr 13, 2018

Uh oh!

charlesdemers commented Apr 13, 2018

Uh oh!

RobbieTheWagner commented Apr 13, 2018

Uh oh!

charlesdemers commented Apr 13, 2018

Uh oh!

RobbieTheWagner commented Apr 13, 2018

Uh oh!

charlesdemers commented Apr 13, 2018

Uh oh!

RobbieTheWagner commented Apr 13, 2018

Uh oh!

adamjmcgrath commented Apr 13, 2018

Uh oh!

adamjmcgrath commented Apr 13, 2018

Uh oh!

kategengler commented Apr 13, 2018

Uh oh!

adamjmcgrath commented Apr 13, 2018

Uh oh!

adamjmcgrath commented Apr 13, 2018

Uh oh!

RobbieTheWagner commented Apr 14, 2018

Uh oh!

adamjmcgrath commented Apr 17, 2018

Uh oh!

RobbieTheWagner commented Apr 17, 2018

Uh oh!

adamjmcgrath commented Apr 17, 2018

Uh oh!

RobbieTheWagner commented Apr 17, 2018

Uh oh!

kategengler commented Apr 17, 2018

Uh oh!

RobbieTheWagner commented Apr 17, 2018

Uh oh!

kategengler commented Apr 17, 2018 via email

Uh oh!

RobbieTheWagner commented Apr 17, 2018

Uh oh!

RobbieTheWagner commented Apr 17, 2018

Uh oh!

RobbieTheWagner commented Aug 30, 2018

Uh oh!

charlesdemers commented Oct 17, 2018

Uh oh!

charlesdemers commented Oct 17, 2018

Uh oh!

RobbieTheWagner commented Oct 18, 2018

Uh oh!

charlesdemers commented Oct 20, 2018

Uh oh!

RobbieTheWagner commented Oct 21, 2018

Uh oh!

JaKXz Jan 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coreyfarrell Jan 6, 2019

Choose a reason for hiding this comment

Uh oh!

kategengler Jan 6, 2019

Choose a reason for hiding this comment

Uh oh!

kategengler commented Jan 6, 2019

Uh oh!

Leooo commented Sep 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

charlesdemers commented Sep 28, 2020

Uh oh!

JaKXz Jan 5, 2019 •

edited

Loading

Leooo commented Sep 28, 2020 •

edited

Loading