Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate with coveralls for code coverage #548

Merged
merged 7 commits into from Nov 11, 2017
Merged

Integrate with coveralls for code coverage #548

merged 7 commits into from Nov 11, 2017

Conversation

jmrodri
Copy link
Contributor

@jmrodri jmrodri commented Nov 10, 2017

Describe what this PR does and why we need it:

Changes proposed in this pull request

  • integrating travis-ci with goveralls (coveralls for golang)
  • generate coverage reports of our code to better understand our testing
  • also added a target for developers to see the coverage locally: make ci-test-coverage

Does this PR depend on another PR (Use this to track when PRs should be merged)
none

Which issue this PR fixes (This will close that issue when PR gets merged)
none

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 10, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d5a4d43 on coveralls into ** on master**.

@jmrodri
Copy link
Contributor Author

jmrodri commented Nov 10, 2017

TEST

  • you can see the output of the coverage quite easily just run make test-coverage-html. A browser will open with the output. That same file is used to feed coveralls.

  • The ci-test-coverage will NOT work locally because it uses the -service travis-ci option which works ONLY in travis. I was able to get goveralls to update coveralls locally but it requires the project API KEY in the command which I didn't want to commit to the repo.

Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

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

The only thing I think needs to be cleaned up are the duplicate lines in the Makefile. I would like if there were a way for you to push the coverage files into a directory that we could simply gitignore for git and rm -rf ${coverage_dir} on cleanup.

Makefile Outdated
@$(foreach pkg,$(PACKAGES),\
go test -coverprofile=coverage.out -covermode=count $(pkg);\
tail -n +2 coverage.out >> coverage-all.out;)
@go tool cover -html=coverage-all.out # -o coverage.html
Copy link
Member

Choose a reason for hiding this comment

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

So if you are excluding -o coverage.html where is the file that I would open in my browser to see it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

running go tool cover -html=coverage-all.out will actually open the browser window for you. With the -o it will simply write it to the file. I'll remove the reference to the coverage.html

Makefile Outdated

ci-test-coverage: ## CI test coverage, upload to coveralls
@echo "mode: count" > coverage-all.out
@$(foreach pkg,$(PACKAGES),\
Copy link
Member

Choose a reason for hiding this comment

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

This is a duplicate from test-coverage-html, is there a way for you to not repeat yourself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will look at deduping

.gitignore Outdated
handler.out
registries.out
validation.out
coverage.html
Copy link
Member

Choose a reason for hiding this comment

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

I don't get the impression that you need this anymore

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 5df6494 on coveralls into ** on master**.

@jmrodri
Copy link
Contributor Author

jmrodri commented Nov 10, 2017

@djzager I have deduped the coverage generation for you. I also removed references to coverage.html since I no longer use it.

@jmrodri jmrodri removed the request for review from eriknelson November 10, 2017 21:30
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9daaca8 on coveralls into ** on master**.

Now the Makefile will monitor coverage-all.out. If the sources get
changed, it will rebuild coverage-all.out assuming you updated code.

Then when you run test-coverage-html or ci-test-coverage, these tasks
will look to see if coverage-all.out is out of date, it not just show
the coverage.
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling a209070 on coveralls into ** on master**.

Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

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

LGTM

@jmrodri jmrodri merged commit c53115e into master Nov 11, 2017
@jmrodri jmrodri deleted the coveralls branch November 11, 2017 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build needs-review size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants