Add test-coverage & codecov target and update circleci

[vdemeester]

This is more or less what we used on some other docker project.

• Not sure if test-coverage should be on his own or be just test
• @dnephin I added parellel here too 👼
• @nandhini915 the codecov token is set on circleci I guess ? 👼

Signed-off-by: Vincent Demeester [email protected]

[dnephin]

Looks like it's failing because of missing dep.

Do you think we need this in the Makefile, or only the circleci?

I think parallel might actually slow it down, because we only get 1 CPU on CI.

I'm going to push some changes and you can let me know what you think.

[nandhini915]

@vdemeester I don't think the token would be required (since docker/cli is a public repo), but yes for private repos we'd setup codecov token in circleci.

[codecov-io]

Codecov Report

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

@@            Coverage Diff            @@
##             master     #109   +/-   ##
=========================================
  Coverage          ?   46.16%           
=========================================
  Files             ?      161           
  Lines             ?    10998           
  Branches          ?        0           
=========================================
  Hits              ?     5077           
  Misses            ?     5631           
  Partials          ?      290

[dnephin]

LGTM

although this now includes some of my changes

[aaronlehmann]

We run codecov on PRs against the swarmkit and distribution repositories. I don't have anything against it. But I also can't recall a time when the coverage information has been useful. We've pushed back against PRs that didn't have adequate tests, but that came from reviewing the PR and noticing that the tests were insufficient. I don't think we've ever made the decision based on the codecov report. In the past we tried to automatically fail CI for PRs that decreased the coverage ratio, and this worked very poorly. There was random fluctuation in the results because of nondeterminism in some code and tests (for example, the order in which two goroutines execute might affect which code paths are taken). It also penalized changes that added extensive error checking, when those errors were not possible to trigger in practice (and therefore the code paths did not show up in code coverage).

cc @stevvooe

[dnephin]

I don't think we have any plans to make it fail CI, I agree that is unreliable.

For me the benefit to this integration is that it makes it easier to see the test coverage for a PR. Test coverage may be easy to see in a PR that adds new features, but test coverage in a PR that refactor lots of code is less obvious.

The valuable metric to me is patch coverage, not total coverage.

[mlaventure]

LGTM

[stevvooe]

Why aren't these just in the Makefile? These kinds of systems are extremely hard to maintain.

[dnephin]

What are "these kinds of systems" ? And what makes them hard to maintain?

[stevvooe]

A mess of bash scripts. It contributes to a horrid build experience and is very hard to modify.

This particular example is absurd. Why is this in its own file?

[stevvooe]

Please include a comment as to why this required.

[stevvooe]

Please address this. Why is this not an argument to the original command?

[dnephin]

What is "this"? The profile.out filename?

https://lk4d4.darth.io/posts/multicover/

[stevvooe]

The file is given as one argument, then copied to the other. Why not give the original file to the command and have it write directly? Are you trying to append it to a global coverage file?

[dnephin]

The file isn't copied. The file is appended to the global coverage file yes. That's what it's doing.

[vdemeester]

LGTM @dnephin's commit 👍

[stevvooe]

@vdemeester I would hope that you would at least address my comments before merging.

This Makefile setup gets a hard REJECT.

[thaJeztah]

@dnephin @vdemeester can this PR be discussed so that things are addressed or consensus reached?

[dnephin]

Yes, I'm still trying to understand the concerns with this PR. It was important to get it merged so that we have coverage on other PRs. If there are things that need to be changed, we can change them.

[thaJeztah]

This didn't make it into 17.06 - moving milestone