[WIP] Enable coverage statistics #5990

wilzbach · 2016-07-31T04:09:43Z

Seems like only a minor hack is required to compile coverage statistics for DMD. Maybe there is a better way to do this, but otherwise it is a very good use case of the new __FILE_FULL_PATH__.

With this PR the coverage is run for both the tests & phobos. We have to see how much it affects the Travis runtime,
Of course as for dlang/druntime#1620, a next step would be to run coverage tests on the AutoTester too.

It can be previewed here.

wilzbach · 2016-07-31T04:43:42Z

With this PR the coverage is run for both the tests & phobos. We have to see how much it affects the Travis runtime

Seems like the factor is ~1.5-2x of the build time for the two jobs.

On the other hand the current documentation says that a job is killed if it "takes longer than 50 minutes on travis-ci.org", so it seems like they silently increased the timeout limit as the builds are still passing.

WalterBright · 2016-07-31T05:56:45Z

What goes into the dmd coverage? Compiling the test suite, compiling Phobos, or both?

Also, while this is great, I'm concerned about slowing down the autotester 2x.

wilzbach · 2016-07-31T06:06:56Z

What goes into the dmd coverage? Compiling the test suite, compiling Phobos, or both?

Both.

Also, while this is great, I'm concerned about slowing down the autotester 2x.

Yes I do know :/
One idea would be to use yet another free CI provider (e.g. CircleCi, Shippable, Wercker, ...) and use them just for code coverage testing. This way we would get at least some coverage statistics without affecting the existing build pipelines. I will explore the options soon, but if someone has a good recommendation, please let me know.

UplinkCoder · 2016-07-31T18:55:55Z

My suggestion would be to only run the coverage once per day on master.

wilzbach · 2016-07-31T19:04:36Z

My suggestion would be to only run the coverage once per day on master.

Well we want to know the effect of a PR on the total coverage, so that won't work, but as stated before I think using another free CI dedicated to compile the coverage statistics seems like a good solution to me.

UplinkCoder · 2016-07-31T19:58:24Z

Just pushing on work on another service is hardly fair.
most PR's will not really care about coverage.

Another suggestion would be to use a magic word such as [cov] in the pr title if it is requested.

dnadlinger · 2016-07-31T20:43:27Z

We can just have the auto-tester report the results, because it already runs all the tests… Oh wait.

codecov-io · 2016-07-31T22:48:37Z

Current coverage is 87.59% (diff: 50.00%)

No coverage report found for master at f0db716.

Powered by Codecov. Last update f0db716...e9181cd

WalterBright · 2016-07-31T23:28:58Z

For now, some sort of rate limiting will be necessary. See below.
Shouldn't include Phobos in determining coverage. Phobos should not be a test suite for the compiler and Phobos should not be coded in a manner that increases dmd test coverage. Changes in Phobos should not raise red flags about reduced dmd coverage.
(2) implies Phobos/Druntime merges should not trigger dmd coverage runs.
Only if the PR itself changes should the coverage run on it happen, not if other merges happen.
Coverage runs should not be done on 'automerge on' runs, because presumably the PR has already been tested and approved, and nobody will be looking at coverage results on automerge runs.

wilzbach · 2016-08-01T00:48:08Z

We can just have the auto-tester report the results, because it already runs all the tests… Oh wait.

Yes but without coverage statistics which is a bit time consuming - on Travis it doubled the build time (second and fourth build):

Just pushing on work on another service is hardly fair.
most PR's will not really care about coverage.

The idea of automation is that you get warned automatically if a new PR introduces lines which are never tested.

Shouldn't include Phobos in determining coverage. Phobos should not be a test suite for the compiler and Phobos should not be coded in a manner that increases dmd test coverage. Changes in Phobos should not raise red flags about reduced dmd coverage.

Fair enough, I assume the same applies for Druntime?

(2) implies Phobos/Druntime merges should not trigger dmd coverage runs.
Only if the PR itself changes should the coverage run on it happen, not if other merges happen.

That's the default behavior for Github ;-)
(I think this wouldn't be possible without our own trigger bot or other services).

Coverage runs should not be done on 'automerge on' runs, because presumably the PR has already been tested and approved, and nobody will be looking at coverage results on automerge runs.

Merge commits have a different commit hash and diff, so I don't know how we could teach CodeCov that. Imho this optimization isn't worth it, because a full build on e.g. CircleCi takes 20 minutes & there are about 3 merges per day.

I think using another free CI dedicated to compile the coverage statistics seems like a good solution to me.

Adding the setup for CircleCi wasn't that difficult and it takes less then 20 minutes for both 32-bit and 64-bit coverage builds to pass (they are run in parallel). Imho that's the best way to move forward as it doesn't strain our burdened resources or has any other negative impact (=it doesn't affect the other builds and if it crashes we only lack coverage info for pending PRs).
Afaik the LDC devs (e.g. @klickverbot?) are using CircleCI too and at mir we have made good experiences with this service too. Apart from the fast setup times, their biggest feature is SSH access to the builds. I doubt this will be needed, but it helped me a lot on other projects.

@WalterBright what do you think? Worth the experiment?

WalterBright · 2016-08-01T03:15:03Z

Fair enough, I assume the same applies for Druntime?

Yes.

That's the default behavior for Github ;-) (I think this wouldn't be possible without our own trigger bot or other services).

I thought dmd runs were done by the autotester if any Phobos merges happen.

What about item (4)?

I found this on CircleCI: https://circleci.com/pricing/

I suppose as long as they offer 1 free container, might as well take advantage of it. So merging this won't slow down the autotester?

WalterBright · 2016-08-01T03:22:55Z

Note that because the back end doesn't have a fully open-source license, we can only use one container free on CircleCI.

wilzbach · 2016-08-01T03:58:31Z

I thought dmd runs were done by the autotester if any Phobos merges happen.

Yes, but all other CI providers just listen to the push events of a PR and consequently put a build into their job queue.

What about item (4)?
Only if the PR itself changes should the coverage run on it happen, not if other merges happen.

Sorry skipped over it. So you refer to merge commits added to the PR or rebases? A forced rebase deletes the previous commit hash and diff, merge commits might be able to be detected.
How often does this happen? I would suggest as this isn't blocking AutoTester nor Travis, we see whether this is really an issue before investing time on fixing it.

I found this on CircleCI: https://circleci.com/pricing/
I suppose as long as they offer 1 free container, might as well take advantage of it.
Note that because the back end doesn't have a fully open-source license, we can only use one container free on CircleCI.

I think they have a similar literal definition of open source as Travis CI that goes like "everything that is publicly available on Github". Paid plans are for private repos.
Hence I think we qualify for four free containers.

So merging this won't slow down the autotester?

(If this change is error-free), AutoTester shouldn't know about it. Coverage is only run if the DMD_TEST_COVERAGE variable set.

FYI I just removed the changes from the travis.yml. If this gets merged, the projects needs to be enabled on CircleCi. This process is similar to Travis.

WalterBright · 2016-08-01T04:31:24Z

I think they have a similar literal definition of open source as Travis CI that goes like "everything that is publicly available on Github". Paid plans are for private repos. Hence I think we qualify for four free containers.

Could you ask them for clarification that that is their definition, please?

MartinNowak · 2016-08-01T20:36:32Z

Why would we start to use another CI provider if we have to run the test suite there again? Doesn't save any resources.

MartinNowak · 2016-08-01T20:40:40Z

src/mars.d

+        version (Windows)
+            enum sourcePath = dirName(__FILE_FULL_PATH__, `\`);
+        else
+            enum sourcePath = dirName(__FILE_FULL_PATH__, '/');


Why is this necessary? Couldn't you just run the script that parses the coverage results from the src folder so that the relative paths work.

Couldn't you just run the script that parses the coverage results from the src folder so that the relative paths work.

Well the script just runs the normal test_runner without modifications, the only change is that dmd has been compiled with -cov.
I am not very familiar with the dmd setup, so if you have better ideas, shoot! ;-)

It's not about the test runner, but about the resulting coverage files (*.lst). Wouldn't it work if you upload them after changing into the src folder? That folder is our compilation root.

If I am not mistaken, If Druntime can't find the source files, it will just produce empty .lst files.

Wouldn't it work if you upload them after changing into the src folder?

It turns out that we don't need to set the dest directory, because the issue was on CodeCov's side and one can manually set mapping patterns.
(the dest directory change is a later follow-up)

If I am not mistaken, If Druntime can't find the source files, it will just produce empty .lst files.

Yes, sounds true.

wilzbach · 2016-08-01T20:42:12Z

Could you ask them for clarification that that is their definition, please?

Yep done, might take a while until we hear back from. I guess until then we can use the 1 container which is free for everyone.

Why would we start to use another CI provider if we have to run the test suite there again? Doesn't save any resources.

It saves our resources, because we don't have to run the test suite with coverage counting enabled (about twice as much time). The idea was to make the coverage statistics independent from the other CI runners, s.t. there is no interference and risk of breaking / slowing down anything.

WalterBright · 2016-08-01T21:41:03Z

Is it ready to go then with the free 1 container?

wilzbach · 2016-08-01T21:45:09Z

Is it ready to go then with the free 1 container?

I would wait for @MartinNowak's feedback on his comment about injecting the source path. Maybe there's a smarter way.

yebblies · 2016-08-02T01:19:15Z

If we're evaluating dmd's coverage, we only need to build the test suite, not run it.

braddr · 2016-08-02T03:58:22Z

Ideally skip the link step too for the runnable category. That'd cut a good amount of time out of the run. Do both of those and the time to add just that step to the existing builds shouldn't be much at all.

WalterBright · 2016-08-02T04:54:56Z

Also, skip permuting the args unless the permuting is explicit in the test case source.

WalterBright · 2016-08-02T20:47:46Z

Auto-merge toggled on

WalterBright · 2016-08-02T20:48:12Z

I want to get this started! Can always improve it.

wilzbach · 2016-08-03T00:48:48Z

Can always improve it.

The ideas to avoid run and link time are quite nice - I honestly didn't think they would result in a huge difference. I had a short look at the test_runner, but it will take me a bit of time to figure out a good way to hack it.

I want to get this started!

@WalterBright thanks! As said before I think you still need to enable CircleCi for dmd ;-)

WalterBright · 2016-08-03T02:37:45Z

The link time is substantial, probably a lot more than the run time.

WalterBright · 2016-08-03T02:46:29Z

I enabled it, and this is as far as it got:

case $CIRCLE_NODE_INDEX in 0) MODEL=32 ./travis.sh ;; 1) MODEL=64 ./travis.sh ;; esac
+ '[' dmd = gdc ']'
+ N=2
+ git clone --depth=1 --branch https://github.com/D-Programming-Language/druntime.git ../druntime
fatal: repository '../druntime' does not exist

case $CIRCLE_NODE_INDEX in 0) MODEL=32 ./travis.sh ;; 1) MODEL=64 ./travis.sh ;; esac returned exit code 128

wilzbach · 2016-08-03T03:03:52Z

I enabled it, and this is as far as it got:

Seems like ed2aaca got into master via #5995 just before. Sorry about that. Proposed fix is at #6003.

MartinNowak · 2016-08-04T10:43:21Z

I didn't got an answer for a critical question here, why should we add yet another CI provider, when there is no need for that?
This means more clicking, more configuration, more troubles.
Right now those tests fail on stable b/c someone forgot to configure CircleCI to only test configured branches.

MartinNowak · 2016-08-04T11:03:34Z

It saves our resources, because we don't have to run the test suite with coverage counting enabled (about twice as much time)

You can just make separate jobs for those, in a similar way that we add those SELF_COMPILE=2 variations. Those can run in parallel to the other tests, no additional CI service needed.

wilzbach · 2016-08-04T11:41:21Z

I didn't got an answer for a critical question here, why should we add yet another CI provider, when there is no need for that? This means more clicking, more configuration, more troubles.

I thought I explained the reasoning

no risk to break Travis (for the beginning breakage of CircleCi tests can be ignored)
no slowdown of Travis (=more resources)

Right now those tests fail on stable b/c someone forgot to configure CircleCI to only test configured branches.

Afaik I can read the error message at #6014, it fails because there is no circle.yml file. Btw a bit related, I opened an issue at CircleCi on how to get the branch against which the PR is run.

You can just make separate jobs for those, in a similar way that we add those SELF_COMPILE=2 variations. Those can run in parallel to the other tests, no additional CI service needed.

Well the maximal parallelization for Travis is 5 (if their queue is empty), so adding more jobs will directly add up to its total run-time and with coverage enabled took between 30 and 50 mins on Travis.

MartinNowak · 2016-08-04T16:02:38Z

Well the maximal parallelization for Travis is 5 (if their queue is empty), so adding more jobs will directly add up to its total run-time and with coverage enabled took between 30 and 50 mins on Travis.

OK, then let's keep it as it is for now.

MartinNowak · 2016-08-05T08:27:57Z

Yikes, CircleCI apparently doesn't merge PRs into the target branch before building them, please be aware of this subtle detail. Both the auto-tester and Travis-CI do this correctly.
https://discuss.circleci.com/t/show-test-results-for-prospective-merge-of-a-github-pr/1662/4

wilzbach force-pushed the master branch from 5b60145 to 2d7cc18 Compare July 31, 2016 04:24

wilzbach force-pushed the master branch from 2d7cc18 to 9dd3b5b Compare July 31, 2016 05:08

wilzbach force-pushed the master branch from 9dd3b5b to de45de0 Compare July 31, 2016 19:00

wilzbach force-pushed the master branch from de45de0 to e6f63f7 Compare July 31, 2016 19:35

wilzbach force-pushed the master branch from e6f63f7 to 1b4a9e1 Compare July 31, 2016 21:36

wilzbach force-pushed the master branch from 1b4a9e1 to 2e6e0e8 Compare July 31, 2016 23:00

wilzbach force-pushed the master branch from 2e6e0e8 to e9181cd Compare August 1, 2016 00:14

Enable coverage statistics

f3cd1d5

wilzbach force-pushed the master branch from e9181cd to f3cd1d5 Compare August 1, 2016 03:48

MartinNowak reviewed Aug 1, 2016
View reviewed changes

WalterBright merged commit d7ba72e into dlang:master Aug 2, 2016

[WIP] Enable coverage statistics #5990

[WIP] Enable coverage statistics #5990

Conversation

wilzbach commented Jul 31, 2016

wilzbach commented Jul 31, 2016

WalterBright commented Jul 31, 2016

wilzbach commented Jul 31, 2016

UplinkCoder commented Jul 31, 2016

wilzbach commented Jul 31, 2016

UplinkCoder commented Jul 31, 2016

dnadlinger commented Jul 31, 2016 • edited Loading

codecov-io commented Jul 31, 2016 • edited Loading

Current coverage is 87.59% (diff: 50.00%)

WalterBright commented Jul 31, 2016

wilzbach commented Aug 1, 2016

WalterBright commented Aug 1, 2016

WalterBright commented Aug 1, 2016

wilzbach commented Aug 1, 2016

WalterBright commented Aug 1, 2016

MartinNowak commented Aug 1, 2016

MartinNowak Aug 1, 2016

Choose a reason for hiding this comment

wilzbach Aug 1, 2016

Choose a reason for hiding this comment

MartinNowak Aug 4, 2016

Choose a reason for hiding this comment

wilzbach Aug 4, 2016

Choose a reason for hiding this comment

MartinNowak Aug 4, 2016

Choose a reason for hiding this comment

wilzbach commented Aug 1, 2016

WalterBright commented Aug 1, 2016

wilzbach commented Aug 1, 2016

yebblies commented Aug 2, 2016

braddr commented Aug 2, 2016

WalterBright commented Aug 2, 2016

WalterBright commented Aug 2, 2016

WalterBright commented Aug 2, 2016

wilzbach commented Aug 3, 2016

WalterBright commented Aug 3, 2016

WalterBright commented Aug 3, 2016

wilzbach commented Aug 3, 2016

MartinNowak commented Aug 4, 2016

MartinNowak commented Aug 4, 2016 • edited Loading

wilzbach commented Aug 4, 2016

MartinNowak commented Aug 4, 2016

MartinNowak commented Aug 5, 2016

dnadlinger commented Jul 31, 2016 •

edited

Loading

codecov-io commented Jul 31, 2016 •

edited

Loading

MartinNowak commented Aug 4, 2016 •

edited

Loading