-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Simplify test coverage collection #2560
Simplify test coverage collection #2560
Conversation
Signed-off-by: Pankhudi Bhonsle <pankhubh@thoughtworks.com>
c8a66aa
to
eb033ab
Compare
Makefile
Outdated
@@ -153,7 +153,7 @@ cover: nocover | |||
@echo pre-compiling tests | |||
@time go test -i $(shell go list ./...) | |||
# TODO Switch to single `go test` that already supports multiple packages, but watch out for .nocover dirs. | |||
@./scripts/cover.sh $(shell go list ./...) | |||
go test ./... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will not produce code coverage results. Try this:
$(GOTEST) -coverprofile cover.out ./...
Signed-off-by: Pankhudi Bhonsle <pankhubh@thoughtworks.com>
Codecov Report
@@ Coverage Diff @@
## master #2560 +/- ##
==========================================
+ Coverage 95.30% 95.31% +0.01%
==========================================
Files 208 208
Lines 9281 9281
==========================================
+ Hits 8845 8846 +1
Misses 357 357
+ Partials 79 78 -1
Continue to review full report at Codecov.
|
Resolves #797 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't know why it happened, but nice to see coverage go up by 0.02% :-)
Makefile
Outdated
@@ -153,7 +153,7 @@ cover: nocover | |||
@echo pre-compiling tests | |||
@time go test -i $(shell go list ./...) | |||
# TODO Switch to single `go test` that already supports multiple packages, but watch out for .nocover dirs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove the above 3 lines. Pre-compilation no longer makes sense since we invoke a single go test
command anyway, and TODO is done by this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also no longer need nocover
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do need it, it's protection against adding packages that don't have test files, which are not included in coverage by default.
Signed-off-by: Pankhudi Bhonsle <psbhonsle08@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
This was my first ever contribution and i did not feel overwhelmed. All thanks to you for your help. :) |
you are welcome, and congrats on the first PR, may it be the first of many. |
Signed-off-by: Pankhudi Bhonsle psbhonsle08@gmail.com
Which problem is this PR solving?
Short description of the changes