chore: use nyc over istanbul-cli #316

JaKXz · 2017-02-10T04:02:45Z

👋 Hi! istanbuljs member here -

Thought I'd help out a little by simplifying your coverage commands and configuration :)

Otherwise, the main win of bringing you over to the much better supported nyc + babel-plugin-istanbul is that you're now getting more accurate coverage reports and stats!

Please let me know if anything concerns you - very happy to answer any questions you might have. Thanks again for this wonderful library!

also add cross-env + babel-plugin-istanbul for better coverage reports

coveralls · 2017-02-10T04:07:08Z

Coverage decreased (-7.7%) to 78.793% when pulling 9deb626 on JaKXz:chore/use-nyc-etc into 3eafa08 on airbnb:master.

coveralls · 2017-02-10T04:36:20Z

Coverage decreased (-7.7%) to 78.793% when pulling 59822ab on JaKXz:chore/use-nyc-etc into 3eafa08 on airbnb:master.

ljharb · 2017-02-10T05:30:30Z

Can you elaborate more on why we should change our coverage method? What's different about nyc and why should we prefer it over istanbul?

ljharb

Additionally, please remove yarn.lock. Airbnb does not use yarn, and lockfiles are harmful in non-top-level-apps.

ljharb · 2017-02-10T05:31:07Z

.editorconfig

@@ -0,0 +1,16 @@
+root = true


unless this file can be kept in sync with CI (ie, changes to it will cause misformatted files to fail) i'd prefer the file not be included.

I have found EditorConfig to be very helpful as a maintainer because it takes care of nuisances with whitespace and line endings and newlines etc. behind the scenes. e.g. the IntelliJ platform comes with support built in and almost all popular editors have a simple installer. It's worth noting that GitHub also has native support.

If you still prefer it not be added, I will remove it.

I'd love to have it, but something in CI would need to enforce it (the linter typically does this for us). Do you know of anything that can do that?

https://github.com/jedmao/eclint looks promising!

But I don't feel like it's a strong enough win for you, if it must be enforced like this.

I will remove it from this PR since it's not really the main focus. 🚲 🏠 painted.

ljharb · 2017-02-10T05:31:42Z

test/components/CalendarDay_spec.jsx

@@ -30,7 +30,7 @@ describe('CalendarDay', () => {
      expect(wrapper.text()).to.equal(lastOfMonth.format('D'));
    });

-    it('contains arbitrary content if renderDay is provided', () => {
+    it.skip('contains arbitrary content if renderDay is provided', () => {


just noting this line will need reverting before merging

ljharb · 2017-02-10T05:32:03Z

test/mocha.opts

@@ -0,0 +1,5 @@
+--compilers js:babel-register,jsx:babel-register


this is a good change regardless; i'd be stoked to merge it in a separate PR :-)

ljharb · 2017-02-10T05:32:22Z

test/mocha.opts

+--require airbnb-js-shims
+--require ./test/_helpers/ignoreSVGStrings.jsx
+--recursive
+test/**/*_spec.js test/**/*_spec.jsx test/_helpers/restoreSinonStubs.js


however, the glob part of this configuration should remain in package.json, not in the mocha opts.

JaKXz · 2017-02-10T05:53:48Z

What's different about nyc and why should we prefer it over istanbul?

nyc is the successor to the istanbul cli. nyc uses istanbul's APIs under the hood and there have been some features & fixes since istanbul@1.x-alpha was published.

It's preferred over the istanbul cli because

the istanbul cli is no longer really maintained, or, better stated: future development is happening in nyc.
babel-plugin-istanbul + nyc provides more accurate coverage of ES2015+ code [hence coverage numbers dropping, relatively signficantly].

ljharb · 2017-02-10T06:06:40Z

package.json

    "pretest": "npm run --silent lint",
-    "tests-only": "npm run mocha --silent test",
+    "tests-only": "mocha --silent",


I think the file path needs to be restored here?

This was a typo on my part - the --recursive flag handles the globs for us.

JaKXz · 2017-02-10T06:23:05Z

@ljharb mind elaborating on

lockfiles are harmful in non-top-level-apps.

If you publish with a files array in package.json you're not adding any weight to the package as a dependency. I notice this package uses .npmignore and I missed adding it there, so that was my bad. Is there something I'm missing?

coveralls · 2017-02-10T06:23:39Z

Coverage decreased (-7.7%) to 78.793% when pulling 5ac50bb on JaKXz:chore/use-nyc-etc into 3eafa08 on airbnb:master.

ljharb · 2017-02-10T06:25:07Z

files arrays are incredibly dangerous, for one, but npmignore is fine.

If I publish a shrinkwrap file, yarn ignores it. If I publish a yarn lock file, yarn and npm ignore it. In no case am I ensuring my users get a deterministic dep tree. However, I am ensuring that if an in-range update is published that breaks me, my own tests will merrily pass but my users will merrily break.

Only an app is in the position to truly know that dependencies should be frozen.

JaKXz · 2017-02-10T06:26:13Z

Curiouser and curiouser

files arrays are incredibly dangerous, for one

Mind elaborating on that? edit: or do you have a slack channel I can bug you on so I can stop digressing here? 😄

coveralls · 2017-02-10T06:31:29Z

Coverage decreased (-7.7%) to 78.793% when pulling 5ac50bb on JaKXz:chore/use-nyc-etc into 3eafa08 on airbnb:master.

ljharb · 2017-02-10T18:25:07Z

@JaKXz i'm on a number of common slacks and on freenode IRC, but here's fine too.

Basically, the absolute worst case of forgetting to include a new file in npmignore is that "your package is a tiny bit too large". The worst case of forgetting to include a new file in "files" is that "your package is subtly broken, in a way that's almost impossible for you, the author, (or your CI tests) to determine". Overarching package size just isn't a big problem anyways, and the worst case is so much worse with an "inclusion list" approach, that the only safe approach is an "exclusion list".

JaKXz · 2017-02-10T21:44:50Z

I see. That's a reasonable approach to safety.

Anyway, are there any other changes you want to see in this PR?

coveralls · 2017-02-10T22:31:46Z

Coverage decreased (-7.7%) to 78.793% when pulling cbb57e5 on JaKXz:chore/use-nyc-etc into cfc7b16 on airbnb:master.

majapw · 2017-02-13T18:30:13Z

@ljharb what do we need to do to get this merged in?

coveralls · 2017-02-13T19:02:30Z

Coverage decreased (-8.0%) to 78.689% when pulling 99b5dd4 on JaKXz:chore/use-nyc-etc into 536563b on airbnb:master.

ljharb · 2017-02-13T23:22:29Z

test/mocha.opts

@@ -0,0 +1,4 @@
+--compilers js:babel-register,jsx:babel-register
+--require airbnb-js-shims
+--require ./test/_helpers/ignoreSVGStrings.jsx


This fails to include the file that restores sinon stubs; please revert this part and restore npm run mocha, and have that pass the file there. mocha doesn't work with global before/after handlers if they're --required in.

ljharb · 2017-03-31T07:32:25Z

@JaKXz i've rebased this branch; but it seems like your previous command (nyc mocha) is a bit wonky; it doesn't seem to take the same path arguments as mocha does; and we prefer to use npm run mocha as our central source of truth for the mocha command. Separately, adding the path args to mocha.opts doesn't seem to work either.

Is this not something nyc supports?

JaKXz · 2017-03-31T23:08:02Z

@ljharb sorry for the long turnaround on this! I think I've addressed your concerns now. :)

I'm not sure why Travis isn't firing but I can confirm the tests pass locally.

ljharb

Awesome, thanks!

Just three more comments/questions, and assuming tests pass, I think this is good to go!

I appreciate your persistence with the back-and-forth and the many comments; I've switched to using nyc on another project and it's both a smooth and pleasant upgrade from istanbul-cli.

ljharb · 2017-04-01T00:47:06Z

package.json

    "lint": "eslint --ext .js,.jsx src test",
-    "mocha": "mocha --compilers js:babel-register,jsx:babel-register --require airbnb-js-shims --recursive ./test/_helpers",
+    "mocha": "mocha ./test/_helpers test/**/*_spec.js*",


is the _spec.js part needed? Every JS file inside test should be considered a test, and double-running the helpers shouldn't be a problem :-/

ljharb · 2017-04-01T00:47:34Z

package.json

    "pretest": "npm run --silent lint",
-    "tests-only": "npm run mocha --silent test",
+    "tests-only": "npm run mocha -- --silent",


the --silent was intended for npm, not mocha - this change passes it to mocha instead. Can this line be reverted?

ljharb · 2017-04-01T00:49:27Z

.nycrc

+  ],
+  "all": true,
+  "check-coverage": true,
+  "statements": 89,


just one final re-confirmation - are these numbers exactly in line with our current coverage? If not, can we bump them to be? :-)

JaKXz · 2017-04-01T01:02:24Z

@ljharb here are the original watermarks, and I've dropped the numbers in .nycrc to reflect the current coverage.

Also addressed your other two comments. Thanks for your patience as well!

coveralls · 2017-04-01T01:03:38Z

Coverage decreased (-7.3%) to 80.176% when pulling a2957c8 on JaKXz:chore/use-nyc-etc into 5385116 on airbnb:master.

coveralls · 2017-04-01T01:15:55Z

Coverage decreased (-7.3%) to 80.176% when pulling fcd7d8d on JaKXz:chore/use-nyc-etc into 5385116 on airbnb:master.

coveralls · 2017-04-01T02:01:42Z

Coverage decreased (-7.3%) to 80.176% when pulling 29f774c on JaKXz:chore/use-nyc-etc into 5385116 on airbnb:master.

majapw · 2017-04-01T03:31:50Z

Party! Why did coveralls coverage go down so significantly though?

JaKXz · 2017-04-01T03:48:49Z

Because the instrumentation is much more accurate now, and we are measuring against all JS and jsx files :) which I don't think was happening before.

majapw · 2017-04-01T03:50:09Z

Exciting! Also, I think I need to write more tests. :P @ljharb is this good? Can we merge it in? I'll lower the coveralls check for now and admin priv merge it, if that's the case.

Thanks for all the hard work @JaKXz! We super appreciate it. :D

ljharb · 2017-04-01T05:04:37Z

I've just re-rebased and pushed up; if travis-ci passes (even if coveralls drops) then this is good to merge 🎉

coveralls · 2017-04-01T19:53:52Z

Coverage decreased (-7.3%) to 80.176% when pulling 5bdcdc4 on JaKXz:chore/use-nyc-etc into 00c95c7 on airbnb:master.

ljharb · 2017-04-01T20:05:21Z

Coveralls passed which probably means the thresholds aren't high enough; but I'm going to merge this now!

ljharb · 2017-04-01T20:05:57Z

(I filed #417 to combine coverage across all test runs, instead of just on node 4.6)

ljharb · 2017-04-01T20:06:22Z

Thanks @JaKXz! 🎉

majapw · 2017-04-01T23:02:42Z

Thanks y'all! @ljharb, I lowered the threshold so all following prs wouldn't be breaking, but yes, i agree that they are too low.

[Tests] use nyc over istanbul-cli

5bdcdc4

also add cross-env + babel-plugin-istanbul for better coverage reports

ljharb requested changes Feb 10, 2017

View reviewed changes

ljharb reviewed Feb 10, 2017

View reviewed changes

ljharb requested changes Feb 13, 2017

View reviewed changes

ljharb force-pushed the chore/use-nyc-etc branch from 99b5dd4 to 5338446 Compare March 31, 2017 07:31

JaKXz force-pushed the chore/use-nyc-etc branch from 5338446 to 5c26650 Compare March 31, 2017 23:06

ljharb approved these changes Apr 1, 2017

View reviewed changes

ljharb force-pushed the chore/use-nyc-etc branch from 5c26650 to 0f9ffc9 Compare April 1, 2017 00:51

JaKXz force-pushed the chore/use-nyc-etc branch from 0f9ffc9 to a2957c8 Compare April 1, 2017 01:00

ljharb force-pushed the chore/use-nyc-etc branch from 29f774c to 797468f Compare April 1, 2017 05:03

ljharb force-pushed the chore/use-nyc-etc branch from 797468f to 5bdcdc4 Compare April 1, 2017 19:36

ljharb merged commit 5bdcdc4 into react-dates:master Apr 1, 2017

JaKXz deleted the chore/use-nyc-etc branch April 13, 2017 01:16

		@@ -0,0 +1,5 @@
		--compilers js:babel-register,jsx:babel-register

chore: use nyc over istanbul-cli #316

chore: use nyc over istanbul-cli #316

Conversation

JaKXz commented Feb 10, 2017 • edited Loading

coveralls commented Feb 10, 2017

coveralls commented Feb 10, 2017

ljharb commented Feb 10, 2017

ljharb 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

JaKXz Feb 10, 2017 • 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

JaKXz commented Feb 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaKXz commented Feb 10, 2017

coveralls commented Feb 10, 2017

ljharb commented Feb 10, 2017

JaKXz commented Feb 10, 2017 • edited Loading

coveralls commented Feb 10, 2017

ljharb commented Feb 10, 2017

JaKXz commented Feb 10, 2017

coveralls commented Feb 10, 2017

majapw commented Feb 13, 2017

coveralls commented Feb 13, 2017

Choose a reason for hiding this comment

ljharb commented Mar 31, 2017

JaKXz commented Mar 31, 2017 • edited Loading

ljharb left a comment • 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

JaKXz commented Apr 1, 2017 • edited Loading

coveralls commented Apr 1, 2017

coveralls commented Apr 1, 2017

coveralls commented Apr 1, 2017

majapw commented Apr 1, 2017

JaKXz commented Apr 1, 2017

majapw commented Apr 1, 2017

ljharb commented Apr 1, 2017

coveralls commented Apr 1, 2017 • edited Loading

ljharb commented Apr 1, 2017

ljharb commented Apr 1, 2017

ljharb commented Apr 1, 2017

majapw commented Apr 1, 2017 via email • edited by ljharb Loading

JaKXz commented Feb 10, 2017 •

edited

Loading

JaKXz Feb 10, 2017 •

edited

Loading

JaKXz commented Feb 10, 2017 •

edited

Loading

JaKXz commented Mar 31, 2017 •

edited

Loading

ljharb left a comment •

edited

Loading

JaKXz commented Apr 1, 2017 •

edited

Loading

coveralls commented Apr 1, 2017 •

edited

Loading

majapw commented Apr 1, 2017 via email •

edited by ljharb

Loading