Add code coverage

[astorije]

I had that in my working directory for a while, always showing in my git status, so I figured I should wrap it up and post it somewhere.

Not sure we want to make it a badge yet considering the number, but that's good information, and the coverage report, in HTML, can be helpful.

At the moment...

=============================================================================
Writing coverage object [/.../lounge/coverage/coverage.json]
Writing coverage reports at [/.../lounge/coverage]
=============================================================================

=============================== Coverage summary ===============================
Statements   : 9.44% ( 144/1526 )
Branches     : 1.79% ( 33/1845 )
Functions    : 9.82% ( 27/275 )
Lines        : 9.45% ( 144/1524 )
================================================================================

[xPaw]

Another damn ignore file 😢

Are we planning on having tests for client javascript, or could the entire client folder be ignored?

[AlMcKinlay]

Did someone say "ignore file"?

https://github.com/YaManicKill/unified-ignore

I feel your pain, @xPaw. Let's get this sorted :-P

Specifically about this, hmmmm ... I'm not against it entirely. But let's not make 100% a target.

[xPaw]

I think coverage for server files is useful to see (not just % report), as we will know which stuff should be test covered.

Instead of istanbul, should we use a service (e.g. coveralls) for this?

[astorije]

Are we planning on having tests for client javascript, or could the entire client folder be ignored?

I think it's sound to keep them in mind. Until this happens (and it might as well not be parseable by test coverage directly without an extra dance), I suggest we don't ignore the whole folder. But again, this is for internal use at this point, for us, so the actual number does not matter as much as the report, browsable locally when you run npm run coverage.

I feel your pain, @xPaw. Let's get this sorted :-P

Not considering the format, which we might indeed be able to standardize by handling separate rules for all of them separately in your repo (which does not simplify anything), most ignore files will have different content for good reasons. So not as easy as it sounds, plus it does sound like over-engineering and a new source of problems we'll have to solve.

@YaManicKill: But let's not make 100% a target.
@xPaw: I think coverage for server files is useful to see (not just % report), as we will know which stuff should be test covered.

My point exactly. Considering our current state, it's more about having a LOC report than a coverage number. Plus, there is quite a gap between 9% and 100% :D

Instead of istanbul, should we use a service (e.g. coveralls) for this?

Coveralls is just a (public) report explorer / badge maker, that relies on whatever (internal) coverage tool we use. See astorije/chai-immutable@c02556d and astorije/chai-immutable@1d2b527 for example. I'd say until we are ready to make the coverage report / percentage more a public-facing metric and less an internal tool, coveralls or others will not be necessary.

[AlMcKinlay]

Not considering the format, which we might indeed be able to standardize by handling separate rules for all of them separately in your repo (which does not simplify anything), most ignore files will have different content for good reasons. So not as easy as it sounds, plus it does sound like over-engineering and a new source of problems we'll have to solve.

Fwiw, I'm not suggesting we try and hack this into the repo, I'm trying to get people behind the idea of a unified ignore file.

If you'll notice, one of the examples was just a yml file that had different sections for different apps, so they can all have different contents, easily.

I'm not really sure how it's "over-engineered" as it's basically just putting all the files into 1 file, with the possibility for a defaults section.

But anyway, this is not the place for discussion of it, we are not going to use it unless it becomes a standard.

[astorije]

If you'll notice, one of the examples was just a yml file that had different sections for different apps, so they can all have different contents, easily.

Well, look at me, I actually did not open that file. I also missed the intent of the project/suggestion and thought you wanted to include this here soon, which as you said, is not possible until it becomes a standard. It's going to be a very long road, and not an easy one, but the goal is very admirable and powerful. OK, closing this discussion then :-)

[AlMcKinlay]

In that case, I'll give me 👍 on this one. I want to keep an eye on this and see what we end up doing with it, because code coverage can become a stick to beat people with. But I'll support it going in juts now.

[maxpoulin64]

I think it's not a bad tool to have even if practically none of us bothers making tests. I agree about the yet another ignore file point, but hey, that's what happens when one adds tooling. Ideally if all of our tools integrated in Mocha like a previous PR I did we could have uniformized that a bit, but it's really just a minor annoyance. Those files also includes some configs, such as eslint rules, csslint rules, so there isn't really much to do around that other than make a script that calls their node APIs instead of their shell commands. 👍

I'll leave open in case @xPaw wants to 👎, otherwise anyone merge.

[xPaw]

Would it work like istanbul cover npm run test:mocha here? I'd rather avoid duplication.

[astorije]

Nop, _mocha != mocha: gotwarlost/istanbul#436

[xPaw]

This does not work Windows (neither does the npm run for that matter). gotwarlost/istanbul#90

(function (exports, require, module, __filename, __dirname) { @IF EXIST "%~dp0\node.exe" (
                                                              ^
SyntaxError: Unexpected token ILLEGAL
    at exports.runInThisContext (vm.js:53:16)

[astorije]

@xPaw, fixed following instructions from https://github.com/gotwarlost/istanbul#usage-on-windows. Can you tell me if it runs fine on Windows now? :-)