Skip to content

Create and send codecov report#34900

Merged
vdemeester merged 2 commits intomoby:masterfrom
dnephin:send-codecov-report
Feb 16, 2018
Merged

Create and send codecov report#34900
vdemeester merged 2 commits intomoby:masterfrom
dnephin:send-codecov-report

Conversation

@dnephin
Copy link
Copy Markdown
Member

@dnephin dnephin commented Sep 19, 2017 •
edited
Loading

Branched from #34911

Create a code coverage report while running unit tests, and upload it to codecov.

This TestNewEnvClient was failing locally so I added some doc strings to debug the failures.

@dnephin dnephin requested review from vdemeester and vieux September 19, 2017 22:21
@dnephin dnephin requested a review from tianon as a code owner September 19, 2017 22:21
vdemeester
vdemeester approved these changes Sep 20, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐯

@AkihiroSuda AkihiroSuda added the status/failing-ci Indicates that the PR in its current state fails the test suite label Sep 20, 2017
@dnephin dnephin added rebuild/janky and removed status/failing-ci Indicates that the PR in its current state fails the test suite labels Sep 20, 2017
@dnephin dnephin mentioned this pull request Sep 20, 2017
Merged
@dnephin
Copy link
Copy Markdown
Member Author

dnephin commented Sep 20, 2017

I'm going to move all the jenkins entrypoint changes to #34911 and rebase this on that branch.

@dnephin dnephin force-pushed the send-codecov-report branch from 0ca00c4 to d8a216d Compare September 20, 2017 21:52
@cpuguy83
Copy link
Copy Markdown
Member

cpuguy83 commented Sep 21, 2017

Can we have two test-unit modes, one with coverage and one without?
The reason being that -cover messes up all the reported line numbers when there are syntax errors.

@dnephin dnephin force-pushed the send-codecov-report branch from 33a4a3b to f6912b5 Compare September 21, 2017 15:05
@dnephin
Copy link
Copy Markdown
Member Author

dnephin commented Sep 21, 2017

Sounds good, I restored the old unit test script, so we can run without coverage.

@dnephin
Copy link
Copy Markdown
Member Author

dnephin commented Sep 21, 2017

build is green! coverage report is at https://codecov.io/gh/moby/moby/commit/f6912b553d7e9f3bf8d11c0218bc0678a9ee216d

@dnephin
Copy link
Copy Markdown
Member Author

dnephin commented Sep 21, 2017

I guess the github integration isn't working yet? Since we don't get a message from codecov on this PR. It may be that the codecov-bash isn't capable of detecting the PR number on jenkins. I'll look into it.

boaz0
boaz0 approved these changes Sep 24, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@boaz0 boaz0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@vieux
Copy link
Copy Markdown
Contributor

vieux commented Oct 3, 2017

@dnephin I believe I create the webhook, can you retrigger / repush ?

@dnephin dnephin force-pushed the send-codecov-report branch from f6912b5 to 8ff99e3 Compare October 4, 2017 15:32
@dnephin
Copy link
Copy Markdown
Member Author

dnephin commented Oct 4, 2017

build is green, but no report :(

@dnephin
Copy link
Copy Markdown
Member Author

dnephin commented Oct 4, 2017

Although there was a banner on https://codecov.io/github/moby/moby/commit/8ff99e35693c5e5a08924baac8b06414e6691881 saying "build is not done yet", and now that banner is gone. So codecov does know that the build finished, it just didn't send a report. Maybe the config has to be in master first?

stevvooe
stevvooe reviewed Oct 4, 2017
View reviewed changes
client/client_test.go Outdated
Copy link
Copy Markdown
Contributor

@stevvooe stevvooe Oct 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a dependency to mark a skipped test?

Copy link
Copy Markdown
Member Author

@dnephin dnephin Oct 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't, but it's better this way. Instead of reporting some meaningless message this prints the condition that returned true, so the literal runtime.GOOS == "windows". Additional messages are also supported, but generally unnecessary.

Copy link
Copy Markdown
Contributor

@stevvooe stevvooe Oct 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a waste of dependency space. The skip message is sufficient.

Copy link
Copy Markdown
Member Author

@dnephin dnephin Oct 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree. Also, this dependency is not being added in this PR, so it's not really relevant.

@stevvooe
Copy link
Copy Markdown
Contributor

stevvooe commented Oct 4, 2017

Does this actually work? On most projects we have running codecov, the numbers are completely wrong.

@dnephin
Copy link
Copy Markdown
Member Author

dnephin commented Oct 4, 2017

It doesn't work yet, but the number in the report https://codecov.io/github/moby/moby/commit/8ff99e35693c5e5a08924baac8b06414e6691881 generally seem plausible.

Which numbers are completely wrong? Coverage percentage?

@stevvooe
Copy link
Copy Markdown
Contributor

stevvooe commented Oct 4, 2017

Which numbers are completely wrong? Coverage percentage?

Usually, the differential percentage. The local coverage tools are very good, but they tend not to take into account how Go does coverage and it skews the numbers.

@cpuguy83
Copy link
Copy Markdown
Member

cpuguy83 commented Jan 16, 2018

What's the status on this?

@dnephin
Copy link
Copy Markdown
Member Author

dnephin commented Jan 16, 2018

Codecov is setup and running, but something is not configured properly, because this PR doesn't show the codecov status.

I'll rebase and see what happens. It might be that the config needs to be in master?

@dnephin
Add code coverage report and codecov config
e5cce50 
Signed-off-by: Daniel Nephin <dnephin@docker.com>
@dnephin
Set git sha from parameter
b2faf24 
Because we merge master into the branch before running tests, so the
actual git sha does not exist on any git remote.

Signed-off-by: Daniel Nephin <dnephin@docker.com>
@dnephin dnephin force-pushed the send-codecov-report branch from 8ff99e3 to b2faf24 Compare January 16, 2018 21:51
@dnephin
Copy link
Copy Markdown
Member Author

dnephin commented Jan 16, 2018

The report is here: https://codecov.io/github/moby/moby/commit/b2faf24925ea0f820db62e1f3d96ee347972f12a (still pending while CI is still running).

@dnephin
Copy link
Copy Markdown
Member Author

dnephin commented Jan 17, 2018 •
edited
Loading

Edit: I spoke too soon, that variable is already set.

@vdemeester
Copy link
Copy Markdown
Member

vdemeester commented Jan 18, 2018

Still LGTM for me 👼

@boaz0
Copy link
Copy Markdown
Contributor

boaz0 commented Jan 18, 2018

LGTM 2

@vdemeester vdemeester merged commit 01bfb6d into moby:master Feb 16, 2018
@dnephin dnephin deleted the send-codecov-report branch February 16, 2018 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stevvooe stevvooe stevvooe left review comments

@vieux vieux Awaiting requested review from vieux

@tianon tianon Awaiting requested review from tianon tianon is a code owner

+2 more reviewers

@vdemeester vdemeester vdemeester approved these changes

@boaz0 boaz0 boaz0 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@stevvooe stevvooe

Labels

area/testing status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@dnephin @cpuguy83 @vieux @stevvooe @vdemeester @boaz0 @thaJeztah @AkihiroSuda @GordonTheTurtle