Coverage at 💯

[MylesBorins]

This change is in two parts
3d553ec which moves the codebase to 100% code coverage. The majority of the work is setting up inline istanbul ignore comments. There is some logic change moving things around and getting rid of dead code... but the bigger changes were broken out.

It is possible some of the changes in the above commit are better suited to be reviewed individually and I'm open to that

2bd60f2 changes the travis.yml file to enforce 100% code coverage for travis to pass.

Perhaps we want to make this a bit more forgiving... perhaps not. We can use this PR to discuss

[gdams]

Why don't we enforce 100% for now and review it again if we find it to be an issue in upcoming pr's

[gibfahn]

So it seems that there are two main reasons to ignore things, windows specific and hard to reach.

For windows specific, are these things that can be dropped once we are building and testing on windows?

I don't currently see the justification for hard to reach coverage exceptions. If we're saying it's extremely difficult to write tests for these code paths, then we're basically saying we don't have/can't reasonably get coverage for them. In which case why not just accept that Coverage isn't 100%? That seems more accurate.

[MylesBorins]

@gibfahn so the main reason I wanted to get the coverage currently to 100% is that it is a number that will not change if we introduce more tests. If we create an arbitrary threshold, that is going to change based on how many files / lines / functions etc... we have in the project.

The windows specific flags are there due to the fact that the CI testing will be done on linux or OSX, thus will never hit those lines. IMHO we shouldn't be sacrificing accurate coverage results for edge cases we know we can't reach during testing

As for the hard to reach coverage exceptions, every single one of them is on code paths that are there to catch run time errors or oddities that "MAY" happen. More than likely the majority of that code could simply be pruned, and we very well may consider doing so. I just felt it was safer to include proper handling for those situation and simply ignore them in the coverage results.

[MylesBorins]

ping @gibfahn

/cc @nodejs/citgm can y'all please take a look and chime in regarding this approach

[gibfahn]

Sorry @thealphanerd for not replying sooner. I've been giving this some thought, and discussing with colleagues.

I understand the aim of having coverage at 100% as a standard, I'm just not sure that the downside of not getting a true figure for coverage checks outweighs the convenience of a number that doesn't change.

Windows

It sounds like @GeorgeAdams95 has the Windows CI stuff pretty much sorted now. When we get windows into CI (which will hopefully be very soon), will it be possible to combine the coverage from different platforms? If not, as we support more platforms will be coverage excluding all non-linux parts of the code?

hard-to-reach

For the hard-to-reach areas, I understand that it's a question of how hard to reach it is, and whether the effort of writing tests outweighs the chance of it ever happening, but I'd still rather see 96% coverage and know we don't need to worry about the other 4% than see 100% coverage and not know what the true figure was.

General

If someone adds more tests, the coverage should never go down right? Are we running the coverage on tests as well? Can we just make a rule in CI that the coverage can never decrease? Obviously we could make exceptions if necessary, but wouldn't that give us the same guarantees as the 100% rule?

If we are going to have hard-to-reach coverage exceptions, I think it'd be worth including an explanation of why we're ignoring them. I guess I need to dig into the code and understand more about why it's so difficult to test them.

[MylesBorins]

@gibfahn most of the "hard to reach" failures in here are to catch edge cases in child_process itself failing. Testing that is extremely difficult.

Other tests are for cases where specific meta data may be missing in objects, we could perhaps find a way to test it, but I have not found a good way just yet

[MylesBorins]

I'm going to drop this for now but revisit again later