bithavoc / express-winston

express.js middleware for winstonjs
https://www.npmjs.com/package/express-winston
MIT License
797 stars 187 forks source link

Convert tests to Mocha with Should #56

Closed floatingLomas closed 9 years ago

floatingLomas commented 9 years ago

As discussed in #54, coverage will have to wait for removing Node 0.6 support because the coverage tools all require >=0.8

Have a look and let me know what you think - there's no rush on this one. If any other PRs get merged in I'll rebase this to include them.

For what it's worth, coverage is currently at 89%, so not too shabby.

bithavoc commented 9 years ago

beautiful. we should add something to the Readme warning: Node 0.6 users should semver to EW 0.1.x, Node 0.8 users should semver to EW 1.0.x

floatingLomas commented 9 years ago

Cool. Once we make the 1.0.x switch and ditch 0.6 I'll add the coverage stuff.

bithavoc commented 9 years ago

Cool. It's supposed to work with IO.js which is Node 1.0 may be we need to add this to the readme from now on?

floatingLomas commented 9 years ago

Travis-CI doesn't support IO.js yet so we can't even test it yet, as per travis-ci/travis-ci#3108. Once that's in place, then it could be added to .travis.yml, tested, then mentioned. Or at least that's how I would approach it.

dougwilson commented 9 years ago

AFAIK istanbul works fine with Node.js 0.6

bithavoc commented 9 years ago

question is: do we really need to support an ancient version of Node.js?

floatingLomas commented 9 years ago

@dougwilson, Istanbul looks like it might - it'll depend on whether its dependencies do.

@bithavoc, good question - is there any way to find out from npm about which node or npm versions are being used to install it?

dougwilson commented 9 years ago

I use it for MySQL coverage on Node.js (https://travis-ci.org/felixge/node-mysql/jobs/47203719) and many other modules that support Node.js 0.6 :)

dougwilson commented 9 years ago

Oh, haha, I take that back; I do not use it on Node.js 0.6, rather I have an exception that skips running istanbul on Node.js 0.6. Wow, can't believe I forgot that :) Basically I have Travis CI run istanbul for non-0.6 builds, haha.

floatingLomas commented 9 years ago

Interesting... I mean, really, only one of the four versions have to run the coverage tests.

I'll give some thought as to how to implement that - which might be made easier if @dougwilson explains how he does it. :P

dougwilson commented 9 years ago

No problem :)

So there are two parts:

  1. In the package.json (or your test runner of choice) I defined multiple ways of invoking the test suite. Usually one for developer, one for developer running coverage, and one for CI (but the CI and coverage can probably just be one thing). Example: https://github.com/jshttp/mime-db/blob/v1.5.0/package.json#L43-L45
  2. Then for actually running the tests, I say "if 0.6, run normal test suite, otherwise run the coverage-generating test suite. Example: https://github.com/jshttp/mime-db/blob/v1.5.0/.travis.yml#L14-L16
  3. The last piece is reporting to coveralls.io (which you may not even want to do): you need to at least not report for 0.6 since no coverage was generated, but you can even do it where it only reports from one version. Example: https://github.com/jshttp/mime-db/blob/v1.5.0/.travis.yml#L17-L18
floatingLomas commented 9 years ago

Awesome. Thanks. :)