-
Notifications
You must be signed in to change notification settings - Fork 3.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
Introduce coverage to PR pipelines #5357
Introduce coverage to PR pipelines #5357
Conversation
9c5f0e2
to
b6cdaa4
Compare
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.
Looks good, but have a couple questions.
PS: once #5368 is merged you should merge in main
to get rid of the unrelated changes to .drone.yml
tools/diff_coverage.sh
Outdated
fi | ||
|
||
echo '```diff' | ||
for pkg in ingester distributor querier ruler storage; do |
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.
Can we make this list configurable in drone? I'd like to surface that there for clarity. This list would need to be amended, but is a pretty good start
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.
What's the reason to only calculate the diff for these packages? I think it would make sense to show all packages.
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.
Also, did you research if there are existing tools to calculate/display coverage diffs based on go test
output?
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.
think all packages could be overwhelming. We want to keep the signal-to-noise ratio high/useful
Although, this is using diff format so it might be enough to look for red lines.
I looked a couple of drone tools but the idea here was:
- something as minimal as possible
- something we control -- we've had security issues with other tools
The next step of this is to make a github comment from a drone job with the diff
Makefile
Outdated
@@ -274,7 +274,10 @@ lint: | |||
######## | |||
|
|||
test: all | |||
GOGC=10 $(GOTEST) -covermode=atomic -coverprofile=coverage.txt $(MOD_FLAG) -p=4 ./... | |||
GOGC=10 $(GOTEST) -covermode=atomic -coverprofile=coverage.txt $(MOD_FLAG) -p=4 ./... | tee test_results.txt |
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.
You need to update the SHELL
variable in the Makefile to use bash -o pipefail
Otherwise the exit code from go test
is not propagated. I ran into the same issue once :D
Makefile
Outdated
GOGC=10 $(GOTEST) -covermode=atomic -coverprofile=coverage.txt $(MOD_FLAG) -p=4 ./... | tee test_results.txt | ||
|
||
compare-coverage: | ||
./tools/diff_coverage.sh $(old) $(new) |
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 might miss something, but where are old
and new
declared?
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.
These are declared as part of the make run: make compare-coverage old=... new=...
This part can't be implemented until we have something to compare with
tools/diff_coverage.sh
Outdated
fi | ||
|
||
echo '```diff' | ||
for pkg in ingester distributor querier ruler storage; do |
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.
What's the reason to only calculate the diff for these packages? I think it would make sense to show all packages.
tools/diff_coverage.sh
Outdated
fi | ||
|
||
echo '```diff' | ||
for pkg in ingester distributor querier ruler storage; do |
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.
Also, did you research if there are existing tools to calculate/display coverage diffs based on go test
output?
e155628
to
3a32889
Compare
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,ruler,storage |
@dannykopping I realized there was no need to wait and implement the comment so I did it today. There's nothing interesting, yet, but future prs should have output like: ./tools/diff_coverage.sh ../tloki/results.txt test_results.txt ruler,ingester,querier + ruler 0
+ ingester 0
+ querier 0 |
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.
Looks great @MasslessParticle! I've added two suggestions, and then LGTM
.drone/drone.jsonnet
Outdated
make('compare-coverage', container=false, args=[ | ||
'old=../loki-main/test_results.txt', | ||
'new=test_results.txt', | ||
'packages=ingester,distributor,querier,ruler,storage', |
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.
'packages=ingester,distributor,querier,ruler,storage', | |
'packages=ingester,distributor,querier,iter,storage', |
The ruler is not super critical here, but the iter
package has far-reaching consequences.
Maybe @cyriltovena can suggest any other critical packages we need to cover if there are any?
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 changed the list to these: ingester,distributor,querier,iter,storage,chunkenc,logql,loki
I think we can be fairly aggressive here because we ought to have color coding to help us see where to focus.
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'd like to also add querier/queryrange.
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.
does it fail if the package name has changed ?
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'd see a large diff, but this won't fail
.drone/drone.jsonnet
Outdated
local make(target, container=true, args=[]) = run(target, [ | ||
'make ' + (if !container then 'BUILD_IN_CONTAINER=false ' else '') + target + ' ' + std.join(' ', args), |
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.
local make(target, container=true, args=[]) = run(target, [ | |
'make ' + (if !container then 'BUILD_IN_CONTAINER=false ' else '') + target + ' ' + std.join(' ', args), | |
local make(target, container=true, args=[]) = run(target, [ | |
std.join(' ', [ | |
'make', | |
'BUILD_IN_CONTAINER=' + container, | |
target, | |
] + args), |
This will get rid of the unrelated changes in drone.yml
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,iter,storage,chunkenc,logql,loki |
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.
LGTM, thanks @MasslessParticle! Excited for this
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki |
Hi! This issue has been automatically marked as stale because it has not had any We use a stalebot among other tools to help manage the state of issues in this project. Stalebots are also emotionless and cruel and can close issues which are still very relevant. If this issue is important to you, please add a comment to keep it open. More importantly, please add a thumbs-up to the original issue entry. We regularly sort for closed issues which have a We may also:
We are doing our best to respond, organize, and prioritize all issues but it can be a challenging task, |
@MasslessParticle following up on this: are we going to land this one? |
@dannykopping I think I've addressed all the feedback? afaik, this is ready to merge. |
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki |
This PR is part one of two to introduce test coverage to our CI pipeline.
Toward #5344
The strategy is to:
test_results.txt
filediff_coverage
will compare the results files and produce a coverage diff for the ingester, distributor, querier, ruler, and storage packages.This PR is for steps 1 - 3. Because we need all branches to produce a
test_results.txt
file, this is a no-op until merged into main. Noop output: https://drone.grafana.net/grafana/loki/8632/1/6Note on
coverage.txt
: The tests already produce a test profile but it only produces per-function results and thego tool coverage -html
is currently broken so it was more sraight forward to search the tests results for package-level results dircetly.