Codecov report generation using ava-nyc #148

suphalb · 2018-01-13T08:09:20Z

This PR will help to generate Codecov Report & Upload it on codecov.io

f2prateek · 2018-01-13T20:14:29Z

hey @suphalb, thanks! can you set this for CI as well? This way the coverage is uploaded to codecov.io (e.g.https://github.com/segmentio/analytics-android/blob/master/.circleci/config.yml#L23)

codecov-io · 2018-01-16T12:31:31Z

Codecov Report

❗ No coverage uploaded for pull request base (master@e1fc2b0). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             master   #148   +/-   ##
=======================================
  Coverage          ?   100%           
=======================================
  Files             ?      1           
  Lines             ?     96           
  Branches          ?      0           
=======================================
  Hits              ?     96           
  Misses            ?      0           
  Partials          ?      0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1fc2b0...c3da3a3. Read the comment docs.

vadimdemedes

The implementation could've been way simpler: https://github.com/sindresorhus/got/blob/master/package.json#L28-L29. There's also no need for coverage-related text to be in the readme.

f2prateek · 2018-01-16T18:21:58Z

circle.yml

@@ -18,3 +18,6 @@ test:
    - nvm use 4; npm rebuild &> /dev/null; npm run testci
    - nvm use 6; npm rebuild &> /dev/null; npm run testci
    - nvm use 8; npm rebuild &> /dev/null; npm run testci
+  post:
+    - npm test
+    - ./node_modules/.bin/nyc report --reporter=lcov > coverage.lcov && bash <(curl -s https://codecov.io/bash)


Should this just be npm run report-coverage (from https://github.com/segmentio/analytics-node/pull/148/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R30)?

@f2prateek . IN fact yes. On my local it's working almost in similar way. But over CircleCI it was not working so had to directlt run that cmd via yml to test if the report is actually generating and uploading. So far it's fulfilling the purpose . But Yes i will make it standard and will find out why it's not running as standard way.

f2prateek · 2018-01-16T18:22:08Z

package.json

@@ -23,10 +23,11 @@
  ],
  "scripts": {
    "size": "size-limit",
-    "test": "standard && ava",
+    "test": "nyc ava tap test.js",


We should still be running standard.

ok i am little confused here . So far i followed #148 (review) and made it minified / simplified. So will you let me know what should i follow here? I meant should i minify it as per @vadimdemedes request OR it will be same like my previous commit?

What I mean is we should still be running the standard command as part of the test.

I believe @vadimdemedes pointed to https://github.com/sindresorhus/got/blob/master/package.json#L28-L29. From there xo = standard (both are code formatting tools).

ok got it clearly @f2prateek . I am taking care of it quickly and coming back with correct commit.

f2prateek · 2018-01-16T18:22:35Z

readme.md

@@ -28,7 +28,18 @@ client.track({

 Documentation is available at [https://segment.com/libraries/node](https://segment.com/libraries/node).

+## Generate Codecoverage Report


No need to add this information to the README.

Yep, Already removed and while i will push my next commit it would be rectified.

f2prateek · 2018-01-16T18:25:52Z

circle.yml

@@ -18,3 +18,6 @@ test:
    - nvm use 4; npm rebuild &> /dev/null; npm run testci
    - nvm use 6; npm rebuild &> /dev/null; npm run testci
    - nvm use 8; npm rebuild &> /dev/null; npm run testci
+  post:
+    - npm test


Do we need to run npm test here? Lines 18-20 are already running a test command as well.

Yes in fact. But again the same problem i experienced . It works on my local but it seems over circle ci it doesn't generate any report. Ideally any JS inside of package.json should be executed via npm test automatically by circleCi. So to test it out i had to run that cmd individually just to ensure if reports are generating. Now i will make it standard .

stephenmathieson · 2018-01-16T21:10:08Z

circle.yml

@@ -8,7 +8,7 @@ dependencies:
    - nvm install 4
    - nvm install 6
    - nvm install 8
-    - rm -rf node_modules
+    - rm -rf 'node_modules coverage .nyc_output coverage.lcov'


Since this is wrapped in quotes, the rm program will try to delete a file called "node_modules coverage .nyc_output coverage.lcov". I suspect this is not what you had intended.

stephenmathieson · 2018-01-16T21:11:22Z

package.json

-    "testci": "standard && ava && npm run size",
-    "prepublish": "npm run check-deps",
-    "check-deps": "nsp check"
+     "test": "standard && nyc ava tap test.js",


What is the word "tap" doing here?

for TAP format report output as console feedback from cmd?

however it's not mandatory but just used for report formation.

I think you need to specify it differently if you want AVA to output TAP: https://github.com/avajs/ava#tap-reporter

stephenmathieson · 2018-01-16T21:12:25Z

readme.md

@@ -22,13 +22,10 @@ client.track({
  userId: 'user id'
 });
 ```
-


We ought to keep a newline between these sections of the README.

stephenmathieson · 2018-01-16T21:52:23Z

circle.yml

@@ -8,7 +8,8 @@ dependencies:
    - nvm install 4
    - nvm install 6
    - nvm install 8
-    - rm -rf 'node_modules coverage .nyc_output coverage.lcov'
+    - rm -rf node_modules ; rm -rf  coverage ; rm -rf .nyc_output ; rm -f coverage.lcov


No need to separate these with semicolons. The single command will work fine.

@stephenmathieson So removed those semicolons and surprisingly this time CircleCI accepted these traditional unix like cmd. Just bit clueless why it didn't get execute with same thing on my previous build before i added those semicolons and inverted commas .

As your suggestion reverted back the readme with it's original state removing all codecov related text etc.

regarding ava tap i followed the reference from here actually - https://github.com/istanbuljs/nyc.
In fact they referred to use it in same way like i did and i have been directed there by codecov nyc example . So as reference they say -

update the scripts in your package.json to include these bins: { "scripts": { "test": "nyc tap ./test/*.js", "coverage": "nyc report --reporter=text-lcov > coverage.lcov && codecov" } } Many testing frameworks (Mocha, Tape, Tap, etc.) can produce TAP output. tap-nyc is a TAP formatter designed to look nice with nyc.

I ran without tap param too however it generates the same output but without a nice serial output formation.

If you think we don't need it here certainly i can omit as it's not mandatory. Please suggest

suphalb · 2018-01-16T22:02:00Z

So in fact it seems things are looking bit different for me over Circle CI. So i tried usual unix style cmd in .yml config such as you mentioned without semicolons . But strange that it even doesn't execute next arguement after the first one as it does on traditional nix/unix shell. I tried it couple of times but on CircleCI console output it doesn't show any effect. Thus CircleCI simply omitted . So i tried with semicolons as well as with inv commas. Second things is same package.json file is being executed without any error on my local stack but yes as it goes through circleCI it throws some error and the build fails . So here i would have to checkout if i am really missing something OR for Circle CI if i need to use some different parameter/method in config file.

…

On Wed, Jan 17, 2018 at 3:22 AM, Stephen Mathieson ***@***.*** > wrote: ***@***.**** commented on this pull request. ------------------------------ In circle.yml <#148 (comment)> : > @@ -8,7 +8,8 @@ dependencies: - nvm install 4 - nvm install 6 - nvm install 8 - - rm -rf 'node_modules coverage .nyc_output coverage.lcov' + - rm -rf node_modules ; rm -rf coverage ; rm -rf .nyc_output ; rm -f coverage.lcov No need to separate these with semicolons. The single command will work fine. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#148 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB8nY9KWMTFU51YaAXAKkXrKhI10ptMDks5tLRoYgaJpZM4RdJPk> .

suphalb · 2018-01-16T22:08:02Z

@vadimdemedes so i simplified & minified as you requested as per the reference , seems that on local it runs without any problem but on CircleCI the build fails as it seems other additional scripts are necessary for CI. In fact for codecov line no 2 & last line is suffice but while it's on CI all other params in scripts section are necessary. I tried 2 builds over CI through more slimmer version like your given reference. However on my local it worked flawless but while it's on CI the build failed. So i got back them again without further refactoring. I would see if there would be any space to refactor it.

…

On Tue, Jan 16, 2018 at 6:10 PM, Vadim Demedes ***@***.***> wrote: ***@***.**** requested changes on this pull request. The implementation could've been way simpler: https://github.com/ sindresorhus/got/blob/master/package.json#L28-L29. There's also no need for coverage-related text to be in the readme. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#148 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB8nY2_dCAf9eCzkVyLvYSqHoo6DsORYks5tLJjBgaJpZM4RdJPk> .

f2prateek · 2018-01-16T22:15:47Z

@suphalb I believe the issue you are seeing is that locally you are running npm test. On CI, we run npm run testci (https://github.com/segmentio/analytics-node/blob/master/circle.yml#L18).

In your PR you only updated the command for npm test to generate coverage reports. But on CI, it runs npm run testci which doesn't generate any coverage reports, and fails the CI with no such file or directory, scandir '/home/ubuntu/analytics-node/.nyc_output' because the coverage report was not generated on the CI.

If you change the testci command to generate coverage reports by changing it to standard && nyc ava tap test.js && npm run size; it should work on CI. (note: i'm not sure if ava tap is correct as per https://github.com/avajs/ava#tap-reporter; but I'm just using this as the example because that's how the test command is currently setup in the PR).

f2prateek · 2018-01-16T22:34:12Z

package.json

@@ -22,11 +22,11 @@
    }
  ],
  "scripts": {
-    "size": "size-limit",
-    "test": "standard && ava",


@suphalb this was accidentally removed, can you put this back?

@f2prateek - can you please consider this latest commit ? b0b7874

Seems this time the CI build was successful with the small tweaks you suggested.

f2prateek · 2018-01-16T22:34:33Z

package.json

-    "testci": "standard && ava && npm run size",
-    "prepublish": "npm run check-deps",
-    "check-deps": "nsp check"
+     "size": "size-limit",


The indentation is off by 1 here, which makes the diff harder to read. Can you leave it at the old indentation level?

vadimdemedes · 2018-01-17T07:41:23Z

@suphalb when you were using an example config I linked to, what was the error on Circle? If it works in other projects, it should as well just work here.

suphalb · 2018-01-17T12:54:39Z

@vadimdemedes - So here it is . Please refer Build test 185 to 188 as following - https://circleci.com/gh/segmentio/analytics-node/185.

It seems if we consider only codecov and report uploading in fact you are correct that it should work and it was working on my Docker. But it seems for CircleCI there have been additional scripts + test run and those were failing if i remove those additional lines and simplify it only for codecov as your exacmple config. So far all other lines were there from earlier and i didn't refactor it completely. All i just added codecov integration with those previous definition. So going through @f2prateek's tweak suggestion i corrected and minified a bit and put everything inside of testCI run.

Please let me know if i am missing anything here?

vadimdemedes · 2018-01-17T13:06:17Z

readme.md

@@ -32,3 +32,4 @@ Documentation is available at [https://segment.com/libraries/node](https://segme
 ## License

 Copyright &copy; 2017 Segment Inc. \<friends@segment.com\>
+


Extra diff.

Will remove it.

vadimdemedes · 2018-01-17T13:06:50Z

package.json

    "prepublish": "npm run check-deps",
-    "check-deps": "nsp check"
+    "check-deps": "nsp check",
+    "report-coverage": "nyc report --reporter=lcov > coverage.lcov && codecov"


=> nyc report --reporter=text-lcov | coveralls

Ok here i am bit confused. I have been informed that we should be using codecov. So am just trying to know will it be codecov integration or coveralls?

My mistake, ignore this comment.

vadimdemedes · 2018-01-17T13:07:27Z

package.json

@@ -23,10 +23,10 @@
  ],
  "scripts": {
    "size": "size-limit",
-    "test": "standard && ava",
-    "testci": "standard && ava && npm run size",
+    "testci": "standard &&  nyc ava tap test.js && npm run size",


Remove tap and test.js.

Ok as i followed codecov i did it based on their reference here what i replied to @stephenmathieson . #148 (comment)

So now i am bit confused about what we are implementing here. Codecov OR Coveralls?

Just let me get back to @f2prateek here about it?

My mistake, we're talking about codecov. However, it's still unrelated to this comment.

well then i will give it a shot again in same way you suggested. Fact is i tried it for couple of times on my Docker stack but it didn't work so i had to get those back on my local. Once it started to work i replicated same over Cir-CI.

It's just ava tap test.js is not a valid command, so I'm not sure how it could work for you.

vadimdemedes · 2018-01-17T13:07:42Z

circle.yml

@@ -8,7 +8,8 @@ dependencies:
    - nvm install 4
    - nvm install 6
    - nvm install 8
-    - rm -rf node_modules
+    - rm -rf node_modules coverage .nyc_output coverage.lcov


No need to remove these new files.

mmm am just wondering in anycase if those folder contain some older reports from different commits/codes etc and then we should not remove those before new testbed "PRE" build step so it can fetch/upload only new & updated reports for the new expected commits/changes/codes etc?

In anycase report-coverage": "nyc report --reporter=lcov > coverage.lcov && codecov this will always create and rebuild those dir/files before producing reports & upload them over codecov.

So i am trying to understand here , those dir/files with old reports & meta data should be still there even with the fresh build for new changes?

Yes, these files will be overwritten in each build anyway, that's why it makes no sense to remove them beforehand.

vadimdemedes · 2018-01-17T13:08:19Z

.gitignore

+coverage
+.nyc_output
+coverage.lcov


No need to ignore coverage.lcov, if you're piping output from nyc to coveralls, like I mentioned in the comment below.

Ok so you mean i should push all of those dir/files with all my local reports & meta data i used for my local test bed? Should not they be ignored like npm rebuild as they will be created always while codecov report generation and uploading via nyc process will be running? Please suggest.

I was talking only about coverage.lcov. But since I was thinking about coveralls and piping output before, it can stay there.

f2prateek · 2018-01-17T17:16:39Z

package.json

@@ -23,10 +23,10 @@
  ],
  "scripts": {
    "size": "size-limit",
-    "test": "standard && ava",


Don't remove the test command (this is still used locally).

stephenmathieson

LGTM!

f2prateek · 2018-01-17T18:13:35Z

circle.yml

@@ -9,6 +9,7 @@ dependencies:
    - nvm install 6
    - nvm install 8
    - rm -rf node_modules
+


Was this line added for a reason? Seems unnecessary and inconsistent with the style.

f2prateek

LGTM! One extra line was added in circle.yml.

f2prateek · 2018-01-17T18:22:36Z

@vadimdemedes all looks good from me and stephen, wanna give your approval too if this looks ok to you?

vadimdemedes · 2018-01-17T18:41:35Z

package.json

-    "test": "standard && ava",
-    "testci": "standard && ava && npm run size",
+    "test": "standard && nyc ava --tap",
+    "testci": "standard &&  nyc ava --tap && npm run size",


What's --tap for? It doesn't affect coverage or reporting.

vadimdemedes · 2018-01-17T18:42:00Z

LGTM from me after #148 (comment) is resolved.

suphalb added 5 commits January 11, 2018 16:02

Codecov report generation using ava-nyc

6eed952

readme.md update

e3d4834

Merge branch 'master' of https://github.com/segmentio/analytics-node

a21ac9c

readme file update

33ea5df

readme file update

2f01d4f

suphalb added 4 commits January 16, 2018 12:27

codecov circleci integration

1764f89

Upstream Update

020955c

circleci config file update

01caa9c

CircleCI integration config update

2f1c798

vadimdemedes suggested changes Jan 16, 2018

View reviewed changes

CircleCI config adjustments

94924b6

f2prateek reviewed Jan 16, 2018

View reviewed changes

review corrections

764b633

stephenmathieson reviewed Jan 16, 2018

View reviewed changes

stephenmathieson suggested changes Jan 16, 2018

View reviewed changes

suphalb added 2 commits January 16, 2018 22:26

review & CI build corrections

415c5c0

CI build fix

da475b6

stephenmathieson reviewed Jan 16, 2018

View reviewed changes

ConfigCI fix

b0b7874

f2prateek reviewed Jan 16, 2018

View reviewed changes

indention fix

877a3f0

readme file revert for space fix etc

fb1babf

vadimdemedes reviewed Jan 17, 2018

View reviewed changes

f2prateek reviewed Jan 17, 2018

View reviewed changes

some mistake fixes

d80ee3b

stephenmathieson approved these changes Jan 17, 2018

View reviewed changes

f2prateek reviewed Jan 17, 2018

View reviewed changes

f2prateek approved these changes Jan 17, 2018

View reviewed changes

f2prateek mentioned this pull request Jan 17, 2018

Replace crypto-token with uuid. #150

Merged

CI yml line formatting

c3da3a3

vadimdemedes reviewed Jan 17, 2018

View reviewed changes

f2prateek mentioned this pull request Jan 17, 2018

Setup for codecov.io #151

Merged

f2prateek closed this Jan 17, 2018

		@@ -28,7 +28,18 @@ client.track({

		Documentation is available at [https://segment.com/libraries/node](https://segment.com/libraries/node).

		## Generate Codecoverage Report

		@@ -32,3 +32,4 @@ Documentation is available at [https://segment.com/libraries/node](https://segme
		## License

		Copyright © 2017 Segment Inc. \<friends@segment.com\>

Codecov report generation using ava-nyc #148

Codecov report generation using ava-nyc #148

Conversation

suphalb commented Jan 13, 2018

f2prateek commented Jan 13, 2018

codecov-io commented Jan 16, 2018 • edited Loading

Codecov Report

vadimdemedes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f2prateek Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suphalb commented Jan 16, 2018 via email

suphalb commented Jan 16, 2018 via email • edited Loading

f2prateek commented Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f2prateek Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

vadimdemedes commented Jan 17, 2018

suphalb commented Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenmathieson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f2prateek left a comment

Choose a reason for hiding this comment

f2prateek commented Jan 17, 2018

Choose a reason for hiding this comment

vadimdemedes commented Jan 17, 2018

codecov-io commented Jan 16, 2018 •

edited

Loading

f2prateek Jan 16, 2018 •

edited

Loading

suphalb commented Jan 16, 2018 via email •

edited

Loading

f2prateek commented Jan 16, 2018 •

edited

Loading

f2prateek Jan 16, 2018 •

edited

Loading

suphalb commented Jan 17, 2018 •

edited

Loading