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

Proposal for coverage report implementation #200

Merged
merged 3 commits into from
Aug 13, 2015

Conversation

ruflin
Copy link
Member

@ruflin ruflin commented Aug 12, 2015

I like it to have in my project a coverage report as part of the build. With the proposed changes below a coverage report is built on codecov.io with every pull request. An example report can be found here: https://codecov.io/github/ruflin/packetbeat?branch=coverage-implementation

During the implementation I stumbled over the issue that a travis build for a fork does not work by default as the package path (github.com/ruflin/packetbeat) inside the GOPATH is incorrect. I adjusted the .travis.yml file so the travis build also works for forks. I removed the line

- ln -s $TRAVIS_BUILD_DIR $HOME/gopath/src/packetbeat

as it didn't seem to be necessary anymore.

Golang can't produce coverage reports for multiple packages at once. This was so far done with the coverage.sh shell script. As this script had some issues on travis I replaced it with the github.com/pierrre/gotestcover go package from @pierrre

I made generating the coverage part of testlong, as I think it makes sense to have it directly inside and brings the advantage that on travis the tests only have be run once.

Please let me know your thoughts on this proposal.

- Make coverage building part of make testlong
- Add sending coverage report to codecov.io
- Implement coverage with github.com/pierrre/gotestcover and remove coverage shell script
@tsg tsg added the review label Aug 13, 2015
@@ -93,15 +95,16 @@ autotest:
.PHONY: testlong
testlong:
go vet ./...
$(GODEP) go test ./...
make cover
make -C tests test
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor comment, but wouldn't make sense to do make cover after the tests? Because I assume cover is slower and doesn't make sense to run it if the tests are failing.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I put it in here is that cover needs to run the tests again. At the moment it doesn't matter much as the test suite runs very fast. I see make test as the fast option to run the tests and make testlong is mainly used on the CI system and to do a final check locally.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, makes sense.

@tsg
Copy link
Contributor

tsg commented Aug 13, 2015

Nice, I like this. I left some minor comments.

We need to make sure this doesn't break our Jenkins builds, but your changes seems to keep all target's functionality, so I think we're good. Anyway, I think the only way to find out is by merging, so I'll do that after you address the comments above.

@ruflin
Copy link
Member Author

ruflin commented Aug 13, 2015

I worried it could break some parts of the Jenkins implementation that is why I kept the changes to the Makefile to a minimum. I will keep an eye on the Jenkins build as soon as it is merged. I assume that is the one to look at? http://build-eu-00.elastic.co/job/packetbeat/

@ruflin
Copy link
Member Author

ruflin commented Aug 13, 2015

Two other things:

  • The travis file has redis-server and elasticsearch inside but I don't think they are used. Could we remove it?
  • I setup for myself a local dev docker image. Would it be interesting to open a pull request with this one? The interesting part here is that in the future with docker-compose an easy environment could be setup to do some integration tests. I assume that was the idea with the redis-server above?

@tsg
Copy link
Contributor

tsg commented Aug 13, 2015

The travis file has redis-server and elasticsearch inside but I don't think they are used. Could we remove it?

Good point. The parts depending on redis & elasticsearch were moved to libbeat and the tests with them. So we can safely remove those from here.

I setup for myself a local dev docker image. Would it be interesting to open a pull request with this one?

Can you make that a different PR, please? Would make sense if it helps to get a dev environment faster.

@ruflin
Copy link
Member Author

ruflin commented Aug 13, 2015

I will open a different PR for the Docker part. Should I remove redis and elasticsearch with this one?

@tsg
Copy link
Contributor

tsg commented Aug 13, 2015

Yes, that would be great, thanks!

@ruflin
Copy link
Member Author

ruflin commented Aug 13, 2015

Done. I will try to open the Dockerfile pull request later today.

@tsg
Copy link
Contributor

tsg commented Aug 13, 2015

You rock, @ruflin! :-)

tsg added a commit that referenced this pull request Aug 13, 2015
Proposal for coverage report implementation
@tsg tsg merged commit 56a23e5 into elastic:master Aug 13, 2015
tsg pushed a commit that referenced this pull request Aug 13, 2015
Needed to keep Jenkins happy. #200
@ruflin ruflin deleted the coverage-implementation branch August 13, 2015 08:26
@ruflin
Copy link
Member Author

ruflin commented Aug 13, 2015

For documentation purpose. Here is the report for elastic/master: https://codecov.io/github/elastic/packetbeat

@ruflin
Copy link
Member Author

ruflin commented Aug 14, 2015

@tsg The Docker implementation is a little bit delayed as the nose tests are sometimes flaky and I'm trying to figure out what the reason is. Is that a common issue?

@tsg tsg removed the review label Aug 17, 2015
tsg added a commit to tsg/beats that referenced this pull request Jan 20, 2016
Proposal for coverage report implementation
tsg pushed a commit to tsg/beats that referenced this pull request Jan 20, 2016
Needed to keep Jenkins happy. elastic#200
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants